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

Release 3.1.0 does not correctly normalize package bins #35

Closed
mpsenn opened this issue Jun 1, 2023 · 2 comments · Fixed by #36
Closed

Release 3.1.0 does not correctly normalize package bins #35

mpsenn opened this issue Jun 1, 2023 · 2 comments · Fixed by #36

Comments

@mpsenn
Copy link

mpsenn commented Jun 1, 2023

npm@9.7.0 was published but never tagged latest due to a bug found during release. below is a root cause of the bug, but the tl;dr is that npm@9.7.0 will be deprecated and npm@9.7.1 will be published once the bug is fixed.

we switched from read-package-json to @npmcli/package-json which recently received an update to bring all the features from the former to the latter. lost in this port was a minor change to normalizing package bins. previously we would not parse package.json#directories.bin if a package.json#bin was present (ref: https://github.com/npm/read-package-json/blob/main/lib/read-json.js#L351-L353) but now we do regardless of whether a bin object is there (ref: https://github.com/npm/package-json/blob/main/lib/normalize.js#L161)

next steps:

  • deprecate npm@9.7.0. this version exists on the registry and contains breaking changes that would impact users if you publish packages using both bin and directories.bin
  • make a fix for this bug in @npmcli/package.json
  • do a further analysis of the changes between read-package-json and @npmcli/package-json and assert this behavior in news tests to ensure no other breaking changes occurred
  • do a new release for npm@9.7.1

Originally posted by @lukekarrys in npm/cli#6470 (comment)

@mpsenn
Copy link
Author

mpsenn commented Jun 1, 2023

Cross-posting this comment to get the issue on the right repo (I think!)

This issue was breaking a build for me, because Google App Engine automatically picks up npm 9.7.0 even though it is deprecated. Working around it by pinning to npm 9.6.7 in package.json.

Running "npm --version"
node:internal/modules/cjs/loader:1078
  throw err;
  ^

Error: Cannot find module '/layers/google.nodejs.runtime/node/bin/node_modules/npm/bin/npm-cli.js'
    at Module._resolveFilename (node:internal/modules/cjs/loader:1075:15)
    at Module._load (node:internal/modules/cjs/loader:920:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

lukekarrys added a commit that referenced this issue Jun 1, 2023
Full list of changes are as follows:
- ignore `directories.bin` if `bin` is present
- walk up filesystem to look for git directory
- log a warning for inaccessible bin entries but dont delete

These changes will be tracked in npm/statusboard#487 to see if they
should be made as breaking changes with npm 10.

These changes are tested by including `read-package-json` as a dev
dependency and running the `test/prepare.js` test suite against both
packages. This test can be removed once we have decided to make these
breaking changes.

Ref: npm/cli#6470
Fixes: #35
lukekarrys added a commit that referenced this issue Jun 1, 2023
Full list of changes are as follows:
- ignore `directories.bin` if `bin` is present
- walk up filesystem to look for git directory
- log a warning for inaccessible bin entries but dont delete

These changes will be tracked in npm/statusboard#487 to see if they
should be made as breaking changes with npm 10.

These changes are tested by including `read-package-json` as a dev
dependency and running the `test/prepare.js` test suite against both
packages. This test can be removed once we have decided to make these
breaking changes.

Ref: npm/cli#6470
Fixes: #35
lukekarrys added a commit that referenced this issue Jun 1, 2023
…ackages

Due to npm/cli#6470, I added `read-package-json` and
`read-package-json-fast` to this repo as dev deps to run the new tests
from #31 against those packages. I found a few discrepancies which I
fixed in this PR.

Full list of changes are as follows:

**`normalize` / `read-package-json-fast`**
- convert `bundleDependencies: false` to `[]`

**`prepare` / `read-package-json`**
- ignore `directories.bin` if `bin` is present
- walk up filesystem to look for git directory
- log a warning for inaccessible bin entries but dont delete

These changes will be tracked in npm/statusboard#487 to see if they
should be made as breaking changes with npm 10.

These legacy tests can be removed once we have decided to make these
breaking changes.

Ref: npm/cli#6470
Fixes: #35
lukekarrys added a commit that referenced this issue Jun 2, 2023
…ackages

Due to npm/cli#6470, I added `read-package-json` and
`read-package-json-fast` to this repo as dev deps to run the new tests
from #31 against those packages. I found a few discrepancies which I
fixed in this PR.

Full list of changes are as follows:

**`normalize` / `read-package-json-fast`**
- convert `bundleDependencies: false` to `[]`

**`prepare` / `read-package-json`**
- ignore `directories.bin` if `bin` is present
- walk up filesystem to look for git directory
- log a warning for inaccessible bin entries but dont delete

These changes will be tracked in npm/statusboard#487 to see if they
should be made as breaking changes with npm 10.

These legacy tests can be removed once we have decided to make these
breaking changes.

Ref: npm/cli#6470
Fixes: #35
lukekarrys added a commit that referenced this issue Jun 6, 2023
…ackages

Due to npm/cli#6470, I added `read-package-json` and
`read-package-json-fast` to this repo as dev deps to run the new tests
from #31 against those packages. I found a few discrepancies which I
fixed in this PR.

Full list of changes are as follows:

**`normalize` / `read-package-json-fast`**
- convert `bundleDependencies: false` to `[]`

**`prepare` / `read-package-json`**
- ignore `directories.bin` if `bin` is present
- walk up filesystem to look for git directory
- log a warning for inaccessible bin entries but dont delete

These changes will be tracked in npm/statusboard#487 to see if they
should be made as breaking changes with npm 10.

These legacy tests can be removed once we have decided to make these
breaking changes.

Ref: npm/cli#6470
Fixes: #35
@mpsenn
Copy link
Author

mpsenn commented Jun 6, 2023

Thanks for the quick fix!

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 a pull request may close this issue.

1 participant