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

Enable npm dedupe as part of Renovate postUpdateOptions #2748

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

klutchell
Copy link
Contributor

@thgreasi
Copy link
Member

thgreasi commented Apr 2, 2024

I think that the tests fail b/c the dependencies in the repo are atm outdated. If you npm i && npm dd && npm i (like the test-dedupe.sh does) and push the diff, then this PR should pass.
An possible alternative I spotted after reading the renovate docs could be to set prefer-dedupe=true in the .npmrc, and will result all npm installs to use --prefer-dedupe, potentially saving us from the extra npm dd command & from augmenting the renovate config.

@thgreasi
Copy link
Member

thgreasi commented Apr 2, 2024

Not entirely sure whether --prefer-dedupe will give the exact same results for the Package lock maintenance PRs, and we might also need this change as well, but I will give it a try on the UI anyway to see how it goes.
See: https://github.com/balena-io/balena-ui/pull/6518

thgreasi
thgreasi previously approved these changes Apr 2, 2024
@klutchell
Copy link
Contributor Author

@thgreasi would we prefer this renovate approach, or the package.json setting used over https://github.com/balena-io/balena-ui/pull/6518? I suspect the latter?

@thgreasi
Copy link
Member

thgreasi commented Apr 2, 2024

@klutchell I think we need to test to which degree the UI PR (--prefer-dedupe) works.
For sure it makes sense when devs do a manual npm i of a new/updated package to have npm to try dedupe upfront (via the npmrc change), but I think we need to wait to check whether that will be enough for renovate PRs as well.

I'm also happy getting this in as is (but you need to do an npm i && npm dd && npm i to match what the CLI does in its tests - I don't know why it uses an extra npm i and it might make sense to remove it even) and wait and compare how those two approaches work for the UI & CLI.

@klutchell
Copy link
Contributor Author

When running rm -rf node_modules && npm i && npm dd && npm i I get the following error:

Failed to apply some patches:
Error: 
**ERROR** Failed to apply patch for package @oclif/core at path
  
    node_modules/@oclif/core

  This error was caused because @oclif/core has changed since you
  made the patch file for it. This introduced conflicts with your patch,
  just like a merge conflict in Git when separate incompatible changes are
  made to the same piece of code.

  Maybe this means your patch file is no longer necessary, in which case
  hooray! Just delete it!

  Otherwise, you need to generate a new patch file.

  To generate a new one, just repeat the steps you made to generate the first
  one.

  i.e. manually make the appropriate file changes, then run 

    patch-package @oclif/core

  Info:
    Patch file: patches/@oclif+core+3.25.0.patch
    Patch was made for version: 3.25.0
    Installed version: 3.26.0

---
patch-package finished with 1 error(s).

@thgreasi
Copy link
Member

thgreasi commented Apr 3, 2024

Let me do that for you @klutchell
See: https://github.com/balena-io/balena-cli/pull/2749/files

@otaviojacobi
Copy link
Contributor

rebasing as this should've been fixed with #2751

@thgreasi
Copy link
Member

I think the issue is that for some reason in the cli the maintainers had chosen to do npm install && npm dedupe && npm install. It might be worth it exploring what dropping the last npm install would result (like we have it in the UI).

@thgreasi
Copy link
Member

@klutchell ends up that you just need to do an npm dd and that is picking up an updated version of @types/node. Just commit that and rebase this and it should be fine.

@klutchell klutchell force-pushed the kyle/renovate-npm-dedupe branch 2 times, most recently from 8de7b72 to e03bd98 Compare April 10, 2024 14:23
@thgreasi thgreasi marked this pull request as draft April 10, 2024 14:29
@klutchell klutchell marked this pull request as ready for review April 10, 2024 14:30
Copy link
Member

Choose a reason for hiding this comment

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

@klutchell it seems that your npm dd reverted the shrinkwrap back to locafileVersion 2. Can you please drop that commit, bump your npm and try npm dd again?

Copy link
Member

Choose a reason for hiding this comment

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

let me do that for you b/c it's been sad how long this PR has taken to get merged...

@thgreasi thgreasi enabled auto-merge April 10, 2024 14:40
@thgreasi thgreasi merged commit 1dbe08d into master Apr 10, 2024
52 checks passed
@thgreasi thgreasi deleted the kyle/renovate-npm-dedupe branch April 10, 2024 15:23
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.

None yet

3 participants