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

feat: explicitly include type #302

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benmccann
Copy link

@benmccann benmccann commented Sep 18, 2024

I've read through several issues in both npm/cli and npm/rfcs and there seems to be universal agreement that adding "type": "commonjs" would be an improvement as it's better to explicit than implicit both for understandability and to improve performance by eliminating the need to go down detection code paths.

Several people have also suggested prompting for the type field and that is much more controversial and has been decided against, so I have not done that.

References

npm/rfcs#75 (comment)

From https://nodejs.org/en/blog/release/v20.10.0:

Detection increases startup time, so we encourage everyone—especially package authors—to add a type field to package.json, even for the default "type": "commonjs".

https://publint.dev/ and the publint package also validate that the type field is present and warn package authors if it is not. It would be best to create a package.json without warnings out of the box: https://github.com/bluwy/publint/blob/master/site/rules.md#use_type

@benmccann benmccann requested a review from a team as a code owner September 18, 2024 16:11
@wraithgar
Copy link
Member

This probably does need a re-evaluation in light of the node 20 behavior.

I don't think it's the best idea to have a value set that the user can't control at least through config. (This comment touches on that, correctly I think).

It's been quite awhile since I looked under the hood on this repo. Would this PR allow npm to pass this in as a defined value (again, not from a prompt but from the --init- config namespace).

@benmccann
Copy link
Author

I'm not quite familiar with the --init- config namespace you're talking about, but this PR won't prevent other values from being set as it only defaults to commonjs if another value is not provided

@wraithgar
Copy link
Member

These are the configs that npm can set, starting at https://docs.npmjs.com/cli/v10/using-npm/config#init-author-email.

The tracking issue for adding the new config is at npm/statusboard#654

@wraithgar
Copy link
Member

I'm adding this to the npm 11 roadmap along with the config changes.

@karlhorky
Copy link

karlhorky commented Oct 1, 2024

Maybe a CC to @arcanis (Yarn) and @zkochan (pnpm) would be good here too?

Feels like a big change in default npm init behavior, and users will probably ask other package managers to follow.

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.

3 participants