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: add import.meta.node.resolveURL #49246

Closed
wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 19, 2023

I'm not really a fan of implementing Node.js specific APIs, but import.meta.resolve is not always fitted for Node.js usage. The main differences are:

  • Async API is so it can be used in custom loader.
  • Returning a URL instance so it's easier to use with node:fs.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Aug 19, 2023
@GeoffreyBooth
Copy link
Member

I don't think we have consensus on the import.meta.node namespace just yet. That part itself needs to go through WinterCG.

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 19, 2023

I don't think we have consensus on the import.meta.node namespace just yet. That part itself needs to go through WinterCG.

IIUC, that's the recommendation from the WinterCG, see #48740 (comment).

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

I don’t think we have consensus on the import.meta.node namespace just yet. That part itself needs to go through WinterCG.

IIUC, that’s the recommendation from the WinterCG, see #48740 (comment).

Maybe that’s the consensus of the WinterCG, maybe it’s not; I don’t really consider that comment summarizing a discussion to be definitive. It’s certainly not a proposal to the WinterCG repo that’s gone through reviews and been accepted. I think such a proposal probably would be accepted, but I think that that needs to happen first before we create this on our side, unless you want to put it behind an experimental flag.

Along those lines, I think we need to discuss import.meta within the TSC and formulate a policy. In particular:

  1. Are additions to import.meta semver-major or semver-minor?

  2. Assuming that WinterCG blesses it, do we want to create an import.meta.node namespace? The advantages are obvious, but it does have a disadvantage in that it encourages more non-portable code to be written. You could make that argument about any node: builtin, though, so I don’t find it all that persuasive in general, though in this particular case it doesn’t add anything that the standardized import.meta.resolve doesn’t already offer (other than the ability to run in the loaders thread; more on that in a moment). Offering a very similar but proprietary API could have the effect of encouraging people to write non-portable code that very easily could have been written in a cross-compatible way. Maybe we shouldn’t care about that either, that we should trust developers to make those decisions, but you could argue that having two ways to do the same thing is bad UX and a footgun.

This PR seems to solve two problems: working with fs APIs more easily by generating URL instances, and providing a way to do resolution within the loaders thread. The first problem already has another possible solution with the import.meta.dirname/filename PR and WinterCG proposal, and I feel confident that that will eventually get approved and land. Which raises the question: if we get import.meta.dirname/filename, do we really need this API as well, for the “make working with Node APIs more convenient” use case? Is it worth adding this when the use case is arguably already satisfied, in a more familiar way, when this solution presents its own UX problems such as being async etc.

The other problem, of being unable to do resolution within the loaders thread, is absolutely something that we should solve. For that, though, I wonder if this is the best solution. A naïve solution that comes to mind is to offer import { resolveAsync } from 'node:module' which would be essentially the same as calling the resolve hook, but async (and requiring a second parameter of parentURL, just like register). It wouldn’t be as ergonomic as this, but the need for resolving within the loaders thread is perhaps not commonplace enough to justify an API so prominently placed as being attached to import.meta, just as we felt that register didn’t deserve such prominence.

Anyway I’m -0 on this overall; the block is just to ensure that this gets discussed within the TSC first and that we resolve the import.meta policy questions before we add anything else to it.

added: REPLACEME
-->

> Stability: 1 – Experimental
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> Stability: 1 – Experimental
> Stability: 1.1 – Active development

Comment on lines +393 to +394
* Returns: {Promise} Fulfills with a {URL} representing the absolute URL of the
resolved specifier.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be confusing that this is async while import.meta.resolve is sync.

Copy link
Member

Choose a reason for hiding this comment

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

perhaps name it import.meta.node.resolveAsync? we have a lot of APIs with a/sync in the name to differentiate that.

Comment on lines +404 to +406
const data = await readFile(await import.meta.node.resolveURL('./data.txt'), 'utf-8');
// This is equivalent to (but also available in custom loaders):
const data2 = await readFile(new URL(import.meta.resolve('./data.txt')), 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

The comment makes it sound like the data2 line, with import.meta.resolve, is available in custom loaders.

Suggested change
const data = await readFile(await import.meta.node.resolveURL('./data.txt'), 'utf-8');
// This is equivalent to (but also available in custom loaders):
const data2 = await readFile(new URL(import.meta.resolve('./data.txt')), 'utf-8');
// This works both in application code and in custom loaders, but is specific to Node.js:
const data1 = await readFile(await import.meta.node.resolveURL('./data.txt'), 'utf-8');
// This is equivalent, and portable to other runtimes, but only works in application code
// (not in custom loaders):
const data2 = await readFile(new URL(import.meta.resolve('./data.txt')), 'utf-8');

Comment on lines +74 to +78
case 'ERR_UNSUPPORTED_DIR_IMPORT':
case 'ERR_MODULE_NOT_FOUND': {
const { url: resolved } = error;
if (resolved) {
return new URL(resolved);
Copy link
Member

Choose a reason for hiding this comment

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

Does import.meta.resolve allow this? I feel like it perhaps should, like it should produce a URL regardless of whether or not the resource exists, but does it? If it’s an intentional variance, and we don’t plan to update import.meta.resolve to match, then we should document this difference.

@@ -0,0 +1,105 @@
// Flags: --experimental-import-meta-resolve
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unnecessary?

Suggested change
// Flags: --experimental-import-meta-resolve

@aduh95 aduh95 closed this May 5, 2024
@aduh95 aduh95 deleted the import-meta-node-resolveURL branch May 5, 2024 08:19
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants