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

esm: allow configuring default package type #32394

Closed
wants to merge 4 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Mar 20, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This is purely to allow people make builds that play around with the idea of changing the default package type, per comments in #32316 . This breaks many expectations and likely should not be done as a runtime flag for now (e.g. the test suite for node cannot run if this this is set to module).

use ./configure --default-package-type
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 20, 2020
@bmeck bmeck added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 20, 2020
src/node_config.cc Outdated Show resolved Hide resolved
src/node_config.cc Outdated Show resolved Hide resolved
bmeck and others added 2 commits March 20, 2020 15:50
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Anna Henningsen <github@addaleax.net>
src/node_config.cc Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Member

Should switching this flag change the name of the binary? Is there a current flag to do so?

I think it might be prudent to not call the binary that is build with this node, I personally would prefer esnode

@bmeck
Copy link
Member Author

bmeck commented Mar 23, 2020

i think node-$mode would be fine, i don't think such a behavior exists currently

@MylesBorins
Copy link
Member

I won't block this PR but I think it would be prudent to force this config option to reconfigure the binary name or we are likely going to get all sorts of weird errors happeninng

@GeoffreyBooth
Copy link
Member

This is purely to allow people make builds that play around with the idea of changing the default package type

I appreciate that you're making this nearly inaccessible to the average user, but why include it in the Node codebase at all? What's the value in a special extra option for someone building their own Node binary? (Not saying there isn't one, but please explain it to me.)

Two issues that occur to me:

  1. Package managers might start shipping an alternate version of Node that's compiled differently. For whatever reason, the Debian maintainers have been shipping their own custom-patched version of CoffeeScript for years now: https://sources.debian.org/patches/coffeescript/1.12.8~dfsg-4/. Granted anyone can do anything with an open source project, but it's just something to be aware of. All the more reason to implement @MylesBorins' suggestion of forcing a different binary name.

  2. What about node_modules? Is there a way to limit the “default package type is module” to stop at a node_modules boundary? Or should we just assume people need to use a script to add "type": "commonjs" recursively to all their "type"-less dependencies under node_modules?

cc @nodejs/modules-active-members

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the discussion, I'd like the public aspect of "type": "none" to not be exposed here.

return pkgType === 'module' || (
pkgType === 'none' &&
defaultPackageType === 'module'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make the default "commonjs" instead of "none" since that is what we've defined to be the default.

@@ -51,7 +52,8 @@ function defaultGetFormat(url, context, defaultGetFormatUnused) {
const ext = extname(parsed.pathname);
let format;
if (ext === '.js') {
format = getPackageType(parsed.href) === 'module' ? 'module' : 'commonjs';
const type = getPackageType(parsed.href);
format = type !== 'none' ? type : defaultPackageType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This defaultPackageType implementation can move into the getPackageType implementation itself. Alternatively getPackageType can return eg undefined to indicate the default should be used.

I just want to avoid explicitly supporting a new "type": "none" value in the public API.

@ljharb
Copy link
Member

ljharb commented Apr 1, 2020

This seems like a very dangerous thing to enable; i recall that the modules group explicitly decided not to do this with discussions around the type flag in the first place.

@jkrems
Copy link
Contributor

jkrems commented Apr 1, 2020

I support this as a build-time flag. I don't think anybody expects this to become something supported by node anytime soon. I'm fine with keeping the name as node. This isn't a build people should have in their PATH long-term. Making that easier, makes it more likely that people do start depending on this feature.

I'm not really concerned that a popular distro would include this (especially if it doesn't affect the binary name). It'll break every node.js user and/or program in the distro. If we want to be explicit, we could name the flag something like --unsupported-and-not-recommended-default-package-type.

What about node_modules? Is there a way to limit the “default package type is module” to stop at a node_modules boundary? Or should we just assume people need to use a script to add "type": "commonjs" recursively to all their "type"-less dependencies under node_modules?

I don't think this is meant to be a realistic feature that people can use right now. It's meant as a playground. Making it easy to use "successfully" is an anti-goal since it may mislead people into thinking this is a build variant we officially support or encourage.

Package managers might start shipping an alternate version of Node that's compiled differently.

I don't think this PR makes this any more or less likely. If they really wanted, they could float a patch to change the default and it would be fairly maintainable to do so. Maintainable as in "keep the patch functional over time".

[...] why include it in the Node codebase at all? What's the value in a special extra option for someone building their own Node binary?

To me the value here is that we have an accessible build with this behavior. PRs and patches bit rot pretty quickly, especially for the module loader code. And by making it a build flag there's a sufficiently high bar that I wouldn't expect it to spread.

@ljharb
Copy link
Member

ljharb commented Apr 1, 2020

Ah, i misunderstood that this is a build time, not a runtime flag; that makes it less dangerous, but I’d still prefer it not to exist; since they can float a patch to “play”, why do they need any more support than that?

@MylesBorins MylesBorins added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Apr 17, 2020
@bmeck
Copy link
Member Author

bmeck commented Apr 22, 2020

In the modules WG meeting we came to a conclusion that we have no intent to change the default package type in the foreseeable future. Given that stance, and that this PR would introduce effectively dead code without the ability to maintain tests for it because of requiring a separate build I will be closing it. If anyone wants to float a patch with the changes made in this PR they can do it on their own fork for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants