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

Update to npm 8 #4763

Merged
merged 3 commits into from
Feb 25, 2022
Merged

Update to npm 8 #4763

merged 3 commits into from
Feb 25, 2022

Conversation

jurre
Copy link
Member

@jurre jurre commented Feb 24, 2022

This updates npm to version 8, which is mostly backwards compatible with v7, but drops support for an old version of node.

We've left the references to npm7 in the codebase (test fixtures and npm version that we reference) as is, because it would become a fairly noisy change and functionally it doesn't change anything. We do plan to follow this up with a PR to update all of those references to npm8.

There is currently one issue that we're digging into which is that the formatting of lockfiles is not respected between updates, and this seems to be caused by a change in npm where in version 8 it bases the formatting (indentation) on the package.json instead. When Dependabot performs updates to the package-lock.json we strip formatting, so in our case this always results in a large diff in the lockfile, however, I don't think we should assume consistent formatting between the package.json and lockfile either, so ideally this is something that can be resolved in npm.

There is one functional change here:

On npm7 (due to a bug in npm), the lockfile would keep it's original formatting (indentation depth, tabs vs spaces) between updates, even if it was different from the package.json. In npm 8, the package-lock.json will always use the JSON formatting from the package.json.

Because of how dependabot updates lockfiles for npm, we modify the JSON in ruby and then spit it back out in a temporary folder. This causes us to lose any formatting, so I've added some code that attempts to figure out how the file was formatted, but the approach is fairly naive, so if someone mixes tabs and spaces, we assume it's tabs, if they mix indentation depth, we use the shallowest indentation. npm will default to indenting with 2 spaces in cases where the is no indentation in the package.json, which I think is preferable in most cases.

I don't expect this to be a huge issue, but it might trip up a few users that rely on this behavior and still use npm 7. We should advise them to update their version of npm because v7 is not officially supported anymore.

jurre and others added 2 commits February 24, 2022 15:41
Co-authored-by: Nishant Sinha <[email protected]>
Previously npm would always return an error when finding peer dependency
conflicts, however in some situations now it merely warns about them.
This handles both cases

Co-authored-by: Nishant Sinha <[email protected]>
@jurre jurre marked this pull request as ready for review February 24, 2022 18:06
@jurre jurre requested a review from a team as a code owner February 24, 2022 18:06
Copy link
Member

@Nishnha Nishnha left a comment

Choose a reason for hiding this comment

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

This is sweet 🍬
Maybe we should do a minor core release for this by itself?
We can suggest users update their version of npm in the release notes.

@jurre
Copy link
Member Author

jurre commented Feb 25, 2022

We can suggest users update their version of npm in the release notes.

Things should work for npm 7, but let's explicitly mention that we're running npm 7 updates using npm 8. Not sure who actually reads our release notes but can't hurt.

Copy link
Contributor

@brrygrdn brrygrdn left a comment

Choose a reason for hiding this comment

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

This approach makes sense to me, I'm fairly sure the 'guessing' of JSON format will have pretty much the same limitations within NPM itself so this implementation seems great! 🚀

indentation_type = indentation.scan(/\t/).any? ? "\t" : " "

indentation_type * indentation_size
end
Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏻 This is one of those functions that's just so satisfyingly terse, I love it

Copy link
Member Author

Choose a reason for hiding this comment

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

Copilot did about half of it to be fair 😄

npm\sERR!\sCould\snot\sresolve\sdependency:\n
npm\sERR!\speer\s(?<required_dep>\S+@\S+(\s\S+)?)\sfrom\s(?<requiring_dep>\S+@\S+)
npm\s(?:WARN|ERR!)\sCould\snot\sresolve\sdependency:\n
npm\s(?:WARN|ERR!)\speer\s(?<required_dep>\S+@\S+(\s\S+)?)\sfrom\s(?<requiring_dep>\S+@\S+)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is handling an internal change between npm 7/8?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it is, in 8 now some peer dependency errors have been turned into warnings

if output.match?(NPM7_PEER_DEP_ERROR_REGEX)
error_context = { command: cmd, process_exit_value: 1 }
raise SharedHelpers::HelperSubprocessFailed.new(message: output, error_context: error_context)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to ensure I understand the change, this is an improvement to attach the command being ran to the error context so it's less of a mystery what has happened?

Copy link
Member Author

Choose a reason for hiding this comment

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

In npm 8 some peer dependency conflicts don't result in an error, but they print a warning out instead and return a 0 exit code. In order to keep the change as minimal as possible this detects whenever such a warning is printed and turns it into an error.

I initially looked into handling those warnings separately but the change ended up being much more involved and harder to follow IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I get it, yeah that makes sense. I agree with this approach of keeping our existing behaviour 👍🏻

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

Successfully merging this pull request may close these issues.

3 participants