Upgrade action to Node v16 and actions/core to v1.10 #68

razor-x · 2023-01-02T04:05:54Z

Passing on my fork: https://github.com/rxfork/npm-publish/actions/runs/3820793379

To use this version while waiting for this PR to merge, you can put

uses: rxfork/npm-publish@v1`

Catch ENOENT error
Fix linter errors
Update Invalid URL assertions
Fix assertions on messages containing registry
Add registry to results
Drop Node 12 support
Update dependencies
Remove Node 10 support comment
Do not fail fast on CI
Update TypeError assertion
Update to @vercel/ncc
Update github actions core
Run using node 16
Add updated test for setOutput
Rebuild

Closes #61, closes #67

razor-x · 2023-01-02T04:24:19Z

@JamesMessinger Just wondering if you are still actively maintaining this, it's really nice work and very well tested! If you need some more help let me know.

Fails with Path contains invalid characters: D:\a\npm-publish\npm-publish\coverage\lcov-report\webpack:\@jsdevtools\npm-publish\src

konstantintieber · 2023-01-25T15:02:42Z

Nice work @razor-x 👍 Hope this will be merged.

hardillb · 2023-01-27T13:40:17Z

@rkrauskopf @JamesMessinger could you please let us know if there will be any further development of this action?

It would be great to get some of the PRs merged to remove the warnings.

I fully understand that commitments and priorities change, if you need help then I'm sure some of us that make use of the action would be willing to help out.

If not then thanks for starting the project, but we need to know so that we should be moving to other options going forward e.g. @razor-x's fork.

hardillb · 2023-03-14T17:01:27Z

It has been month and a half without a comment from @rkrauskopf or @JamesMessinger and 10 months since there have been any commits to the project.

I think we have to assume this project is abandoned now.

@razor-x would you be willing to tag a version on your fork that matches your latest set of PRs that we can reference in Actions (rather than a moving target of latest that may [hopefully] change over time)?

razor-x · 2023-03-20T06:43:57Z

@hardillb Actually I had already tagged v1 for this: https://github.com/rxfork/npm-publish/releases/tag/v1. This should be a stable tag to use as I don't have plans to make changes to this fork unless we run into more issues or warnings.

I'd be happy to take over as maintainer here and open things up to more maintainers if @rkrauskopf or @JamesMessinger would take notice of this thread. We can try their email and link back here. I'd much prefer to a keep this repo or transfer it rather than move to a fork.

mcous

Howdy! Thanks for your work getting the various issues with this repo sorted out. I'm a new maintainer here, and I'd love to get this work in and released ASAP. I've got a few minor notes and questions; please let me know if I can be of any help!

.github/workflows/CI-CD.yaml

mcous · 2023-04-07T15:19:50Z

package.json

@@ -39,14 +39,13 @@
    "test": "mocha && npm run lint",
    "coverage": "nyc -x test -x dist/sourcemap-register.js node_modules/mocha/bin/mocha",
    "upgrade": "npm-check -u && npm audit fix",
-    "bump": "bump --tag --push --all && git tag -afm v1 v1 && git push --tags --force",


Why was this script removed? It looks like the "release" script below relies on it. Should both bump and release be removed?

It's been so long since I looked at this I can't remember. Maybe an issue I hit when trying to develop on a fork if I had to guess.

I think these scripts were calling out to https://github.com/JS-DevTools/version-bump-prompt. I think it's fine to remove the scripts (and the dev dep below on @jsdevtools/version-bump-prompt) because:

It's redundant with the vanilla npm version command

CI will run tests before allowing a release to happen

Any package under @jsdevtools is relatively unlikely to see any maintenance

mcous · 2023-04-07T15:20:01Z

package.json

  },
  "devDependencies": {
-    "@actions/core": "^1.2.6",
+    "@actions/core": "^1.10.0",


src/action/index.ts

test/fixtures/mocha-hooks.js

src/npm-publish.ts

mcous · 2023-04-07T18:14:50Z

@razor-x I just merged #73 to get CI green so this PR can focus on resolving the deprecation issues. Let me know if you'd like me to take over and resolve the merge conflicts to get this in!

razor-x · 2023-04-07T18:27:09Z

@mcous So glad to see this project has an active maintainer again!

I was about to close this PR now, as I cannot commit the time to resolve the new conflicts resulting from your recent changes. Personally, I would have preferred if you have giving me the opportunity to respond to your comments first to merge this contribution, however if you would like to take over the branch and resolve the conflicts we can see what changes are left that may still be valuable to integrate.

In the meantime, I will shift focus to the feature in my other PR: #69

mcous · 2023-04-07T18:34:56Z

Personally, I would have preferred if you have giving me the opportunity to respond to your comments first to merge this contribution, however if you would like to take over the branch and resolve the conflicts we can see what changes are left that may still be valuable to integrate.

My apologies for steamroller! I'll push the conflict resolution up and get this PR in. My company relies on this action to publish about a half-dozen npm packages, and the deprecations will cause those workflows to fail in less than two months, so I was feeling a little stressed and wanted to move as quickly as possible 😅

I'll make sure to request your review(s) moving forward

mcous · 2023-04-07T18:41:07Z

@razor-x ok JK turns out I can't just push to your branch. Opened a PR instead so we can get this PR synced up... 💫 rxfork#1

Merge changes from main

razor-x

@mcous I marked some changes with ⭐ that look valuable. Let me know what you think. For the test changes, up to you if think they are still relevant. Let me know and I can clean up the PR to only include the things we want to keep.

razor-x · 2023-04-07T22:47:47Z

.github/workflows/ci-cd.yml

@@ -21,7 +21,7 @@ jobs:
    runs-on: ${{ matrix.os }}
    timeout-minutes: 10
    strategy:
-      fail-fast: true
+      fail-fast: false


razor-x · 2023-04-07T22:47:50Z

action.yml

@@ -61,5 +61,5 @@ outputs:
    description: The version number that was previously published to NPM

 runs:
-  using: node12
+  using: node16


razor-x · 2023-04-07T22:47:57Z

package.json

    "_eslint": "eslint \"**/*.@(js|ts)\"",
    "_prettier": "prettier \"**/*.@(js|ts|json|md|yml)\""
  },
  "engines": {
    "node": ">=16"
  },
  "devDependencies": {
-    "@actions/core": "^1.2.6",
+    "@actions/core": "^1.10.0",


razor-x · 2023-04-07T22:48:22Z

src/npm-publish.ts

@@ -35,6 +35,7 @@ export async function npmPublish(opts: Options = {}): Promise<Results> {

  let results: Results = {
    package: manifest.name,
+    registry: options.registry,


⭐ But could be moved to other PR.

I think it's a consistent change, given that we may use a default value for registry. Does it also need to be added to the action output?

razor-x · 2023-04-07T22:49:21Z

test/specs/action/failure.spec.js

@@ -9,6 +9,8 @@ const { EOL } = require("os");

 describe("GitHub Action - failure tests", () => {
  it("should fail if the NPM token isn't set", () => {
+    files.create([{ path: "workspace/not-package.json", contents: {} }]);


⭐ I think this is important, but can't remember why anymore.

Removing this line has no effect on the tests for me. Maybe it isn't needed anymore?

razor-x · 2023-04-07T22:50:47Z

src/action/index.ts

+/**
+ * Set output with logging to stdout for test support
+ */
+function setOutput(...args: Parameters<typeof setOutputActionsCore>) {


I think I needed this to make the tests backwards compatible with how the new setOutput works. If things are working without it now maybe we don't need it anymore.

From my testing it seems like we don't need it locally, but we almost certainly need it in CI. Let's keep it! Otherwise we'd have to fiddle with the GITHUB_OUTPUT env var or something.

Later, I'd like to refactor the tests to avoid calling @actions/core directly

mcous

Looks good to me! @razor-x I just realized I have permission to add maintainers so I'm going to add you, too, based on your maintenance of the working fork while this repo was unmaintained. Plus that way with two of us we're a little more protected from maintenance gaps

mcous · 2023-04-08T12:42:36Z

src/action/index.ts

+/**
+ * Set output with logging to stdout for test support
+ */
+function setOutput(...args: Parameters<typeof setOutputActionsCore>) {


From my testing it seems like we don't need it locally, but we almost certainly need it in CI. Let's keep it! Otherwise we'd have to fiddle with the GITHUB_OUTPUT env var or something.

Later, I'd like to refactor the tests to avoid calling @actions/core directly

mcous · 2023-04-08T12:43:47Z

src/npm-publish.ts

@@ -35,6 +35,7 @@ export async function npmPublish(opts: Options = {}): Promise<Results> {

  let results: Results = {
    package: manifest.name,
+    registry: options.registry,


I think it's a consistent change, given that we may use a default value for registry. Does it also need to be added to the action output?

mcous · 2023-04-08T12:44:21Z

test/specs/action/failure.spec.js

@@ -9,6 +9,8 @@ const { EOL } = require("os");

 describe("GitHub Action - failure tests", () => {
  it("should fail if the NPM token isn't set", () => {
+    files.create([{ path: "workspace/not-package.json", contents: {} }]);


Removing this line has no effect on the tests for me. Maybe it isn't needed anymore?

mcous · 2023-04-08T14:36:30Z

package.json

@@ -39,14 +39,13 @@
    "test": "mocha && npm run lint",
    "coverage": "nyc -x test -x dist/sourcemap-register.js node_modules/mocha/bin/mocha",
    "upgrade": "npm-check -u && npm audit fix",
-    "bump": "bump --tag --push --all && git tag -afm v1 v1 && git push --tags --force",


I think these scripts were calling out to https://github.com/JS-DevTools/version-bump-prompt. I think it's fine to remove the scripts (and the dev dep below on @jsdevtools/version-bump-prompt) because:

It's redundant with the vanilla npm version command

CI will run tests before allowing a release to happen

Any package under @jsdevtools is relatively unlikely to see any maintenance

razor-x added 5 commits January 1, 2023 21:10

Catch ENOENT error

74b17c0

Fix linter errors

cd54bcc

Update Invalid URL assertions

e2ee35c

Fix assertions on messages containing registry

8cc8e27

Add registry to results

ddae832

razor-x marked this pull request as ready for review January 2, 2023 04:11

razor-x mentioned this pull request Jan 2, 2023

Add target option #69

Closed

razor-x added 2 commits January 1, 2023 22:38

Run using node 16

1631f87

Update github actions core

3f09b5f

razor-x force-pushed the fix-ci branch from aacbe23 to 3f09b5f Compare January 2, 2023 04:57

razor-x added 8 commits January 1, 2023 23:03

Update to @vercel/ncc

f0817db

Update TypeError assertion

8395ea5

Do not fail fast on CI

8d97f0f

Remove Node 10 support comment

d5e5236

Update dependencies

2a4c66f

Drop Node 12 support

08faf4c

Ensure workspace exists in test

9bce14f

Rebuild

cfaedeb

razor-x force-pushed the fix-ci branch 8 times, most recently from 1e230fb to 7415f4b Compare January 2, 2023 06:27

Add updated test for setOutput

f2c1984

razor-x force-pushed the fix-ci branch 2 times, most recently from 6b200d7 to df18bcb Compare January 2, 2023 06:38

razor-x force-pushed the fix-ci branch from df18bcb to e44a799 Compare January 2, 2023 06:39

Do not run test coverage on windows

721a57b

Fails with Path contains invalid characters: D:\a\npm-publish\npm-publish\coverage\lcov-report\webpack:\@jsdevtools\npm-publish\src

razor-x force-pushed the fix-ci branch from e44a799 to 721a57b Compare January 2, 2023 06:40

razor-x changed the title ~~Fix CI~~ Fix CI and deprecations Jan 2, 2023

This was referenced Jan 2, 2023

Upgrade Action to use node 16 instead of node 12 #61

Closed

Deprecation warning for 'set-output' #67

Closed

rnmeow pushed a commit to rnmeow/x-markdown-css that referenced this pull request Mar 1, 2023

chore(ci): ref. JS-DevTools/npm-publish#68

ea73260

mcous reviewed Apr 7, 2023

View reviewed changes

mcous mentioned this pull request Apr 7, 2023

Fix CLI's --registry option and ensure tests pass in CI #73

Merged

Merge branch 'master' into fix-ci

6f0090a

Merge pull request #1 from JS-DevTools/rxfork-fix-ci

8b7eb6f

Merge changes from main

razor-x commented Apr 7, 2023

View reviewed changes

mcous approved these changes Apr 8, 2023

View reviewed changes

mcous changed the title ~~Fix CI and deprecations~~ Upgrade action to Node v16 and actions/core to v1.10 Apr 8, 2023

mcous merged commit 1641d03 into JS-DevTools:master Apr 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade action to Node v16 and actions/core to v1.10 #68

Upgrade action to Node v16 and actions/core to v1.10 #68

razor-x commented Jan 2, 2023 •

edited by mcous

Loading

razor-x commented Jan 2, 2023

konstantintieber commented Jan 25, 2023

hardillb commented Jan 27, 2023

hardillb commented Mar 14, 2023

razor-x commented Mar 20, 2023

mcous left a comment

mcous Apr 7, 2023 •

edited

Loading

razor-x Apr 7, 2023

mcous Apr 8, 2023

mcous Apr 7, 2023

mcous commented Apr 7, 2023

razor-x commented Apr 7, 2023

mcous commented Apr 7, 2023

mcous commented Apr 7, 2023

razor-x left a comment

razor-x Apr 7, 2023

razor-x Apr 7, 2023

razor-x Apr 7, 2023

razor-x Apr 7, 2023

mcous Apr 8, 2023

razor-x Apr 7, 2023

mcous Apr 8, 2023

razor-x Apr 7, 2023

mcous Apr 8, 2023

mcous left a comment

mcous Apr 8, 2023

mcous Apr 8, 2023

mcous Apr 8, 2023

mcous Apr 8, 2023

Upgrade action to Node v16 and actions/core to v1.10 #68

Upgrade action to Node v16 and actions/core to v1.10 #68

Conversation

razor-x commented Jan 2, 2023 • edited by mcous Loading

razor-x commented Jan 2, 2023

konstantintieber commented Jan 25, 2023

hardillb commented Jan 27, 2023

hardillb commented Mar 14, 2023

razor-x commented Mar 20, 2023

mcous left a comment

Choose a reason for hiding this comment

mcous Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous commented Apr 7, 2023

razor-x commented Apr 7, 2023

mcous commented Apr 7, 2023

mcous commented Apr 7, 2023

razor-x left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

razor-x commented Jan 2, 2023 •

edited by mcous

Loading

mcous Apr 7, 2023 •

edited

Loading