-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Conversation
use ./configure --default-package-type
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Anna Henningsen <github@addaleax.net>
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 |
i think |
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 |
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:
cc @nodejs/modules-active-members |
There was a problem hiding this 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' | ||
); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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. |
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 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
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.
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".
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. |
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? |
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. |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesThis 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
).