fix(manger/npm): apply config.npmrc during extraction, not in post-update

[simon-abbott]

Changes

This moves the application of the global npmrc config option from the post-update phase to the extract phase.

Context

Sub-packages in monorepos sometimes have their own .npmrc files. These should not be shared between packages. This fixes a particularly nasty bug, reported in #12891.

Here's what is actually happening to cause #12891:

1. Create a monorepo using Lerna
2. Make two sub-packages foo and bar
3. Add a .npmrc file to foo with the contents legacy-peer-deps=true
4. Have an out-of-date dependency in foo
5. Run Renovate

When Renovate runs it will execute branchifyUpgrades(), which, in turn, calls flattenUpdates, which will return a branch config object with { npmrc: "legacy-peer-deps=true" }. Later on in the process that config is passed to writeExistingFiles for each package, which dutifully creates a .npmrc file with legacy-peer-deps=true in each subpackage. This is the real bug, since now legacy-peer-deps has been turned on for package bar, even though it's only defined in foo. Then, when the updates are finished Renovate runs lerna bootstrap to update the lockfiles. When Lerna runs with legacy-peer-deps=true it regenerates the lock files, likely because the dependency resolution graph is different since it can meet peer requirements more easily.

To fix this issue, we now apple the npmrc config when extracting per-package-file configs, and write the result to disk writeExistingFiles.

Closes #12891

Documentation (please check one with an [x])

• I have updated the documentation, or
• No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

• Code inspection only, or
• Newly added/modified unit tests, or
• No unit tests but ran on a real repository, or
• Both unit tests + ran on a real repository

[simon-abbott]

Another approach I thought of to fix this would be to always set the packageFile.npmrc setting in extractPackageFile, and only use that in writeExistingFiles.

[viceice]

this needs a unit test

[viceice]

Another approach I thought of to fix this would be to always set the packageFile.npmrc setting in extractPackageFile, and only use that in writeExistingFiles.

that would probably the better solution.

[simon-abbott]

that would probably the better solution.

It shall be done.

this needs a unit test

Agreed, I just wasn't entirely sure how to do that, but doing the other approach would make that easier as well I think.

[simon-abbott]

Force-pushed because this is a complete rewrite of the patch.

[viceice]

looks good so far. please create some sample repos to do some real tests. this change is risky to break something.

[simon-abbott]

looks good so far. please create some sample repos to do some real tests. this change is risky to break something.

What do you mean by that? I didn't see any repos as part of the test suite, did I miss something?

EDIT: If you mean that I should test it on real repos, I have done that. I tested extensively against our private repo before pushing the changes, and I have confirmed that it fixes the issue described in #12891, and that the --npmrc and --npmrcMerge CLI flags both still work as intended (and thus that their config file equivalents will also still work). If that's not what you meant, then I don't fully understand what you're asking.

[viceice]

yes, real repo tests was my intention. thanks for confirming.

[renovate-release]

🎉 This PR is included in version 34.107.1 🎉

The release is available on:

• GitHub release
• 34.107.1

Your semantic-release bot 📦🚀