Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use npm v7 #304

Merged
merged 1 commit into from
Mar 30, 2021
Merged

feat: Use npm v7 #304

merged 1 commit into from
Mar 30, 2021

Conversation

danez
Copy link
Contributor

@danez danez commented Dec 8, 2020

I updated npm from v6 to v7. Reading the blogposts there shouldn't be any breaking changes, at least not in the functionality that semantic-release uses.

One test failed, but it was testing npm internal stuff about formating of package.json, so I removed it. (npm v7 does seem to preserve whatever formatting is in the file and just replace the version.) Hope that is okay?

@danez
Copy link
Contributor Author

danez commented Dec 8, 2020

Not sure why the integration tests fail with 403 errors.

@gr2m
Copy link
Member

gr2m commented Dec 8, 2020

Thanks a lot Daniel, I really want us to upgrade to npm 7, it hopefully get rid of several outdated packages that we are stuck with because of npm 6. I'll try to have a look at the failed CI. This week is busy for me because of GitHub Universe, but I'll do my best. I definitely have it on my list and won't forget

@ext
Copy link

ext commented Feb 13, 2021

Possible a bit off-topic but I am a bit curious as to why it has to be a dependency at all, is not not possible to use system npm?

@gr2m
Copy link
Member

gr2m commented Feb 17, 2021

not possible to use system npm?

the problem is compatibility. WE don't know what system npm version you have and making sure that the code is compatible with all current and future version is a maintenance nightmare

@ext
Copy link

ext commented Feb 17, 2021

not possible to use system npm?

the problem is compatibility. WE don't know what system npm version you have and making sure that the code is compatible with all current and future version is a maintenance nightmare

But it is also a bit nightmare-ish for users? It pulls many extra dependencies (some with security vulnerabilities) and it also causes incompatibilities in run scripts, e.g. system with npm 7 uses npm 6 (or vice versa after this PR) when using npm in a run script because node_modules/.bin/npm in now in $PATH. (Bonus points when node_modules/.bin/npm says there is an update to NPM despite the system already runs the latest)

But I get your point, I was mostly curious about the reasoning.

@lattwood
Copy link

lattwood commented Mar 29, 2021

Expect this issue to have more activity in the coming days, we received this notification two hours ago from dependabot.

edit: Link to GHSA: GHSA-vx3p-948g-6vhq / CVE-2021-27290

image

@danez
Copy link
Contributor Author

danez commented Mar 30, 2021

@gr2m I figured out why the integration tests fail, It is only because npm v7 does not set the maintainers field anymore on publish, but npm-registry-couchapp requires it to be set.

To fix this I switched to the proper npm registry verdaccio instead of the docker image from here.

Now all the test seems to work :)

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work! Thank you so much! Just one question


// Verify the logger has been called with the version updated
t.deepEqual(t.context.log.args[0], ['Write version %s to package.json in %s', '1.0.0', cwd]);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this test?

Copy link
Contributor Author

@danez danez Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it was testing npm internal stuff. Npm v7 does not format package.json files anymore, so this test failed because no newlines were present in the expected package.json.
I could have "fixed" the test and adjusted the expected output, but the test before this one (Preserve indentation and newline) does exactly the same thing then and the testname would have not really described anymore what it is testing. So I decided to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thanks 👍🏼

@gr2m gr2m merged commit a15c017 into semantic-release:master Mar 30, 2021
@danez danez deleted the npm7 branch March 30, 2021 12:40
@github-actions
Copy link

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wyardley
Copy link

This won't result in projects still using the old format publishing with the new package-lock.json format, right?

This may not technically be breaking, but it is causing issues for some of our tooling that was calling npm and expecting the v6 output.

@danez
Copy link
Contributor Author

danez commented Mar 31, 2021

Is the package-lock.json now released with v7? I thought it is always excluded.

@weaintplastic
Copy link

weaintplastic commented Apr 1, 2021

Hey @danez. 👋 Hope you are doing well.

We've updated semantic-release/npm from 7.0.10 to 7.1.0 and since then our release flow isn't able to pubish packages anymore to our private repository due to an authentification error. The release is performed inside a Github action providing anNPM_TOKEN

- name: Release
  env:
    GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
    NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
  run: npm run release -- --debug

The error we're receiving is

npm ERR! code ENEEDAUTH 
1869npm ERR! need auth This command requires you to be logged in. 
1870npm ERR! need auth You need to authorize this machine using `npm adduser`

Reverting the dependency update made our releases to finish successfully again. Do you have any advice where to look for a fix?

@danez
Copy link
Contributor Author

danez commented Apr 1, 2021

hey @weaintplastic,

That is odd as this error only appears if no credentials are found for the registry. https://github.com/npm/cli/blob/latest/lib/publish.js#L99-L109
And semantic release should provide valid credentials for the command.

what is the npm version that is now installed in your project?
What was the complete output of semantic release?
I guess you specify the private registry in package.json->publishConfig->registry?

@weaintplastic
Copy link

@danez we are using node 15 and npm 7.

we have a .npmrc file in our root that includes the read-only NPM-TOKEN.
The token with publishing rights is provided through a Github secret as posted above.
The registry is defined as you said in package.json/publishConfig/registry.

travi added a commit to semantic-release/semantic-release that referenced this pull request Sep 3, 2021
which required swapping the registry from the integration tests to verdaccio, similar to the change
in semantic-release/npm#304

for #2055
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants