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

Support npm 7 in npm source #341

Merged
merged 4 commits into from
Feb 15, 2021
Merged

Support npm 7 in npm source #341

merged 4 commits into from
Feb 15, 2021

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Feb 13, 2021

closes #340

With NPM 7, the npm list command is changed to no longer return the full dependency tree unless --all is specified. This branch/PR checks whether to add the argument to the list command based on the output from npm -v, parsed and compared using Gem::Version.

I'm updating the CI workflows first to run npm CI tests on node 14 and 15, where 15 includes npm 7 by default. I'm expecting that without the changes, we'd see tests on indirect dependencies to fail with node 15 / npm 7.

test on node 14
test on node 15/ npm 7
use actions/setup-node@v2
@jonabc
Copy link
Contributor Author

jonabc commented Feb 14, 2021

😞 this turned into a much larger change than I expected it to. Along with needing the --all flag, there are a few other changes I found that were needed

  1. npm list --long no longer includes homepage and description in the returned data 😢 . Why? Who knows, probably performance considerations. To retain that information I need to load and parse each dependency's package.json file
  2. Missing dependencies raise an error where they used to just be marked as missing. Accounting for this by enhancing the error message when the result from npm list ... can't be parsed to include the returned error message. The implementation isn't amazing but it avoids needing a refactoring or extension of the shell execution code.

@jonabc
Copy link
Contributor Author

jonabc commented Feb 15, 2021

Many of the CI checks are still failing, but node15/npm7 is successful. Going to merge this though I'm still not in love with all the checks for how to handle different npm versions. This might see some iteration in the future.

@jonabc jonabc merged commit c1f7476 into master Feb 15, 2021
@jonabc jonabc deleted the support-npm-7 branch February 15, 2021 04:17
jonabc added a commit that referenced this pull request Mar 24, 2021
## 2.15.0
2021-03-24

### Added
- Support for npm 7 (#341)

### Fixed
- Files in the manifest source will be found correctly for apps that are not at the repository root (#345)
@jonabc jonabc mentioned this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix npm source for npm 7+
1 participant