Next.js incorrectly patches package-lock.json version 3

[remcohaszing]

Verify canary release

• I verified that the issue exists in Next.js canary release

Provide environment information

Operating System:
  Platform: linux
  Arch: x64
  Version: #202204271406~1651504840~22.04~63e51bd SMP PREEMPT Mon May 2 15:
Binaries:
  Node: 16.15.0
  npm: 8.9.0
  Yarn: 1.22.18
  pnpm: N/A
Relevant packages:
  next: 12.1.6
  react: 18.1.0
  react-dom: 18.1.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

There are 3 package-lock.json versions. Version 1 uses the dependencies field. Version 3 uses the packages field. Version 2 combines both for backwards compatibility.

Next.js assumes the dependencies field as the source of truth with a fallback to an empty object, but this is absent in package-lock.json version 3. It will then make unnecessary requests to the npm registry and wrongfully strip the dev field for each dependency

The logic for this is in packages/next/lib/patch-incorrect-lockfile.ts.

Expected Behavior

Next.js patches the lockfile packages field only if the packages field is incomplete and not make unnecessary requests.

To Reproduce

recma-nextjs-static-props is a minimal Next.js project. and can be used as a project. Clone it, regenerate the lockfile, git add it

git clone git@github.com:remcohaszing/recma-nextjs-static-props.git
cd recma-nextjs-static-props
rm package-lock.json
npm install
git add .

Now run npm run dev to see the lockfile is being changed by Next.js.

npm run dev
git diff

[balazsorban44]

Looks like #36813 partially resolved this. When using 12.1.7-canary.4, I don't get a diff on the lockfile before and after in a minimal reproduction, but it still prints warn - Found lockfile missing swc dependencies, patching.. so we should fix that.

The dev field issue is because in your reproduction, next and react/react-dom are added to devDependencies while they should be under dependencies.

[jdharvey-ibm]

Just curious: Why was the decision made by NextJS to modify the lock file at all? (as opposed to just providing remediation steps to the user)? This seems like a dangerous game to play given:

1. You're hand-parsing and hand-writing the package lock file as opposed to using npm commands to modify it.
2. npmjs.com is hardcoded, which is not necessarily the registry from which a project might be pulling its dependencies (e.g. Artifactory, GH Packages, private registries, etc.).
3. It introduces overhead by causing NextJS to need to track changes to the npm ecosystem
4. It modifies a user's "source of truth" dependency file without asking for their permission before doing so (i.e. it is a side-effect of an unrelated command).

It seems like an all around better idea to just not do this, report an error, and move on.

[ijjk]

@jdharvey-ibm this is a very tricky bug for the user to fix depending on their npm version and we don't want to put users down a path of failure so we aim to correct issues where we can. In this case npm is very hard to get to re-add all necessary optionalDependencies once they have been dropped in some cases requiring the lockfile be completely regenerated and wipe all node_modules which loses a lot of context and isn't safe for some projects.

Ideally the bug gets fixed upstream in npm but that leaves users who aren't yet updated in a bad state still which is undesirable.

[jdharvey-ibm]

Thanks for the reply! Though I don't agree with the approach, I can understand the motivation and that your goals with the framework and its adoption are likely much different than mine as a single user.

I would still like to advocate for the ability to opt-out of this behavior (lock file modification of any kind). Perhaps with an environment variable which causes the app to abort with an error as opposed to modifying the lock file. Similar to how telemetry can be disabled.

At IBM, it's fairly common for us to rely on npm repos outside of npmjs for security reasons, so the lack of configurability of this corrective action is a bit unfortunate.

What would be the best way for me to pursue an enhancement of that sort? It sounds like this is falling more into the realm of a feature request issue?

[ijjk]

I opened a PR here #36959 to ensure the different lockfile versions are handled, thanks @remcohaszing for describing the differences there and also added an env variable NEXT_IGNORE_INCORRECT_LOCKFILE to allow skipping this check altogether.

[ijjk]

Hi, this has been updated in v12.1.7-canary.7 of Next.js, please update and give it a try!

[github-actions]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.