-
Notifications
You must be signed in to change notification settings - Fork 31.3k
esm: implement import.meta.main #32223
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
base: main
Are you sure you want to change the base?
Conversation
b6a8c89
to
c7bf59b
Compare
What would the value be in a Worker thread? |
Good question! With the current implementation, it is |
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.
I'm against this for reasons outlined in the issue
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.
I might be convinced otherwise if there's overwhelming support for this feature but I have some concerns about this pattern (see issue comments). My concerns are broadly:
- Usually
import.meta
doesn't change based on how and when the module has been imported. It's already "magic ephemeral data", let's not make it more dynamic and magic. - This pattern encourages writing code that's inherently untestable without setting up a custom module loader from scratch.
- (More optional preference) I would vastly prefer a solution that properly encloses optional code that is meant to run as main and doesn't also allow sprinkling that behavior into random expressions. One example I suggested was to export a main or startup function for this purpose.
@jkrems regarding your first point, having a way to tell if current module is the entry point is a motivation clearly stated in the I think your second point meets @devsnek's: using import { isMainThread } from 'worker_threads';
if (!isMainThread || !import.meta.main) {
process.emitWarning(new Error('This module should be run from CLI.'));
} Regarding your third point, while it'd fine with me, I fail to see how exporting a |
i think package exports can disallow importing your cli entrypoint. I also believe that "you've imported the wrong file" isn't specific to cli entrypoints, there are lots of files that libraries have that shouldn't be directly imported, which is why we should seek more general solutions. |
Boolean value to check if a module is run as entry point of the current process. Fixes: https://gtihub.com/nodejs/modules/issues/274
c7bf59b
to
382d66c
Compare
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.
I’m good with this, personally.
8ae28ff
to
2935f72
Compare
Do we want to reconsider this? Asking because it's been recently brought up in #34882, it seems there are users looking for it. |
The use of The other benefit of I think treating any new context as supporting One pattern I also really like with export function lib () {
// ...implementation..
}
if (import.meta.main) {
assert.strictEqual(lib('unit'), 'test');
} where the tests are run by just running During a build, @jkrems perhaps that counters your untestable argument in (2) :) I do like the idea of having an exports condition, but that seems complementary to this proposal. Would seeing progress on that help overcome your and @devsnek's concerns here? |
What I think is very unfortunate about those kinds of tests, is that they'll never run in a browser without building. So it's effectively creating a test pattern that is browser-incompatible. Not something that node has to care about but I would prefer if we wouldn't encourage the creation of node-only (or bundler-only) code.
It's still "untestable" with the exception of exactly this case: The code inside of the block is a test itself, will only ever run as a subprocess, and doesn't require any testing/assertion framework (block-scoped
Afaik deno is still much smaller as an ecosystem than nodejs (let alone the browser). So adding a module system feature to nodejs because deno is doing it when the feature cannot be used in browsers (likely in perpetuity), isn't really convincing to me. All that being said: These are all ecosystem concerns, not nodejs concerns. I'd be willing to dismiss my block to this change if there's a way to import a module as main without spawning a subprocess. That would make it somewhat testable at least and enable simple wrapper binaries. |
Technically, it could be run on the same process using
I suppose the web could also implement If I'm being honest, the same argument applies to |
I don't think that's true. It's fairly simple to run a file using import {main} from './entrypoint.js';
main(); There's no need to parse & transform |
Yes sorry, I meant the fact that the HTML standard might implement I think you're making a valid point with the browser compatibility thing, and I don't have any argument to counter that 😅 |
Ah yes, sorry. Misread your comment about native browser support. :) |
@jkrems The build step is about removing Example:Input:// src/foo.js
if (import.meta.main) {
throw new Error(`${import.meta.url} is not an entrypoint module`);
}
export function foo() { /* ... */ } // src/main.js
import { foo } from "./foo.js";
export * from "./foo.js";
if (import.meta.main) {
console.log(foo());
} Output:// dist/bundle.js
// src/foo.js
function foo() { /* ... */ }
// src/main.js
if (import.meta.main) {
console.log(foo());
}
export { foo }; |
Yes, that's the problem. In the example above (where the thing inside of |
@jkrems it will likely be down to something like VM / compartments / realms to allow defining |
I think that really doesn't address the usability concerns, just like loader hooks in node wouldn't. E.g. let's say somebody wrote two worker entrypoints, both using I get that the current UX ( Maybe there could be an API like |
please ship this. it's a huge help for a problem countless people run into regularly. allegedly it has risks and/or may not be 100% perfect for every use case (frankly i don't understand most of the concerns) but this seems straightforward & super helpful in 99.99% cases. not shipping it is really hurting us all very much. if we need a more foibled fancy alternative, let it emerge over time on some other import.meta. give us this straightforward common sense help which we very much need now. |
Perhaps we aren't clear on the meaning of |
@guybedford i'm still fundamentally opposed to this feature. even among people who want it, there is disagreement about what qualifies as "main". With TLA you could do |
If act of precisely specifying this feature has identified situations where the value of |
One of the semantics of |
Why though? If someone is writing a custom loader, it doesn't seem very complicated to replace |
I've written custom entry point wrappers in the past that didn't involve any source transformation. Setting the bar at "needs to do contextual compilation of all source code" seems excessive, compared to the status quo. It also makes it impossible to use It's fine if everybody agrees that this is an acceptable regression in the name of replicating the same API "look". But I think it's a meaningful difference that makes this not "just like" |
This sounds distinct from what is being discussed in #53882. It might even be a separate property like |
I'm not saying it's the solution do that problem, I'm just saying it was referenced, and the sources for the feature (this) are seemingly stale, so I wanted to verify they were still being worked on. (Sorry if I worded my last comment poorly) |
I think since Deno supports this property we should add it if we can. I think we should go through the same process with the WinterCG registry that we went through for |
(AFAICT there are no specific issues/PRs blocking this issue) |
The block is because of the still unresolved request for changes on the PR; it should remain. |
The blocked label is specific for issues/PRs blocking something, so I was trying to declutter. Feel free to re-add if needed. |
I dont have the ability to re-add it; it’s also blocked on WinterCG though. |
Is it? AFAICT |
That’s just documentation; it’s not the same as WinterCG having agreed on an interoperable implementation. |
Correct, the |
@aduh95 as authored here is it set to true when importing the main entry point from a worker? |
And just to be clear, my previous approval still stands. I wonder if we can get an update from @jkrems? |
It still feels like an awkward design choice because of things like "can't easily wrap" but those gaps could be filled with loaders or better core APIs in the future. If nobody else has pushed a better API in X years, there's a good chance that making it easier to write more interoperable code is worth it. Would be great to have some canonical definition of the exact semantics that can be shared across runtimes. But worst case, that's what major versions are for..? Willing to dismiss my review here to unblock progress. |
It's been 5 years. It's unlikely that a better alternative materializes.
@@ -66,11 +68,12 @@ function initializeImportMeta(meta, { url }) { | |||
// Alphabetical | |||
if (experimentalImportMetaResolve) | |||
meta.resolve = createImportMetaResolve(url); | |||
meta.main = Boolean(this?.isMain); |
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.
meta.main = Boolean(this?.isMain); | |
meta.main = !!this?.isMain; |
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.
Is there a performance merit to this? or is it for brevity / for consistency? (out of interest)
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.
Both, and it avoids a primordial.
@@ -80,7 +83,9 @@ translators.set('module', async function moduleStrategy(url) { | |||
debug(`Translating StandardModule ${url}`); | |||
const module = new ModuleWrap(url, undefined, source, 0, 0); | |||
moduleWrap.callbackMap.set(module, { | |||
initializeImportMeta, | |||
initializeImportMeta: isMain ? | |||
FunctionPrototypeBind(initializeImportMeta, { isMain }) : |
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.
FunctionPrototypeBind(initializeImportMeta, { isMain }) : | |
() => initializeImportMeta({ isMain }) : |
i believe an arrow is faster than binding
Is it all good if I pick up from here in a new PR? It seems a bit unfair to expect @aduh95 to be keeping on top of it after 5 years 😂 |
Boolean value to check if a module is run as entry point of the current
process.
ES module authors need a way to determine if their code is run as CLI or as a dependency. #49440 have some proposal, I am implementing
import.meta.main
because:import.meta
proposal.Fixes: #49440
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes