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

[DOCS] Possibly undesired and undocumented side-effects from #4917 #5001

Closed
2 tasks done
pvanagtmaal opened this issue Jun 9, 2022 · 6 comments
Closed
2 tasks done
Labels
Documentation documentation related issue Needs Triage needs review for next steps

Comments

@pvanagtmaal
Copy link

pvanagtmaal commented Jun 9, 2022

Is there an existing issue for this?

  • I have searched the existing issues

This is a CLI Docs Problem, not another kind of Docs Problem.

  • This is a CLI Docs Problem.

Description of Problem

Since #4917, workspace .gitignore files are taken into account. However, this means that if you have a typescript workspace, you likely have *.js files in your .gitignore and these will suddenly not be in the packlist anymore.

We have a monorepo, structured as follows:

├── .gitignore             # includes **/*.js and **/*.js.map
├── package.json           # private, to define the workspace
├── backend/
│        ├── package.json  # typescript project, published
│        ├── lib/
├── client/
│        ├── package.json  # typescript project, published
│        ├── lib/

Before npm 8.11.0, a npm pack for either backend or client would include the *.js and *.js.map files from lib, but since 8.11.0 only *.d.ts and index.js are packed. (I am not entirely sure why index.js is packed, I doubt it is relevant for this issue)

Potential Solution

I would have at least expected the change to be

  1. a major change so our CI would behave normally until we would do a major update (with the appropriate testing)
  2. documented properly (either here or in npm-packlist)

Affected URL

No response

@pvanagtmaal pvanagtmaal added Documentation documentation related issue Needs Triage needs review for next steps labels Jun 9, 2022
@louis-bompart
Copy link

Hey @pvanagtmaal , that won't solve your issue, but I had this issue too and I discussed it with one of the npm developers.
There's an explanation of the whys, and resolutions steps (I do agree why you that the changes were surprising)

npm/npm-packlist#106

(mostly commenting because I think this comment npm/npm-packlist#106 (comment) is particularly insightful and could be taken into account for the documentation update)

@pvanagtmaal
Copy link
Author

Hi @louis-bompart,

Thanks for the link, that comment does indeed explain a lot. In our case there were indeed too many files ignored and we fixed it by adding a root .npmignore "unignoring" the js and js.map files.

I do however still feel that this is something that should be in major releases.

@brandonthomas
Copy link

brandonthomas commented Jun 13, 2022

We ran into this issue unexpectedly where our lib and lib-commonjs were in our gitignore in a monorepo. Suddently on update, our lib was no longer included in our package. It didn't break lib-commonjs because we defined a main field which then caused pack to walk up the tree and include lib-commonjs. To me this behavioral change is so big that it should be breaking. Took us a bit to figure it out last week.

We wound up defining files in our package.json to fix the problem.

I fully agree with @pvanagtmaal that this should be a major breaking change.

@AlanSl
Copy link

AlanSl commented Jun 22, 2022

We had the exact same issue as @brandonthomas - on bundles released with NPM >= 8.11.0 the ESM build but not the CJS build was unexpectedly excluded from the bundle.

It's a particularly nasty breaking change because, depending which NPM version consumers of our package are using, they might hit build errors, or (worse) they might not notice any change at first but encounter hard-to-spot bugs (e.g. from multiple builds of mutual dependencies) and inflated bundle size in production builds due to their bundler silently choosing the CJS build over the expected but missing ESM build.

Our workaround was adding negation lines like !lib to the workspace's .npmignore to force those to be not-ignored.


At minimum, if NPM includes the directory specified in the package.json "main" key, it should also include any directory specified in a package.json "module" key if there is one.

@ljharb
Copy link
Collaborator

ljharb commented Jun 22, 2022

No, it shouldn’t, because the “module” key is not a standard thing - it’s just something a few bundlers invented.

@fritzy
Copy link
Contributor

fritzy commented Jun 29, 2022

@pvanagtmaal Noted that you believe this should have been a breaking change. This behavior is already documented in https://docs.npmjs.com/cli/v8/commands/npm-publish

Closing, but feel free to re-open if you have a specific place you'd like us to publish this information.

@fritzy fritzy closed this as completed Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation documentation related issue Needs Triage needs review for next steps
Projects
None yet
Development

No branches or pull requests

6 participants