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: implement import.meta.resolve and make nodejs: scheme public #31032

Closed
wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Dec 19, 2019

This PR implements import.meta.resolve as an async hook into the module resolver.

This fills the gap of enabling require.resolve workflows that aren't currently possible in ES modules, such as locating the exact path of a dependency or local resolution following all the correct resolver resolution rules.

This is a feature that will require alignment with browsers, so I'm opening this now to be able to start those discussions. Marking as blocked on consensus discussions.

Usage examples:

  • await import.meta.resolve('./dep.ext') -> file:///path/to/dep.ext, relative to current module.
  • await import.meta.resolve('./dep.ext', 'file:///different/path/parent') -> file:///different/path/dep.ext, relative to provided parent.
  • await import.meta.resolve('dependency') -> file:///path/to/node_modules/dependency/main.js, as resolved from current module.
  • await import.meta.resolve('dependency', 'file:///different/path/parent') -> file:///different/path/node_modules/dependency/main.js, as resolved from provided module.
  • await import.meta.resolve('./does-not-exist') -> Throws not found as the default Node.js resolver checks for existence.
  • await import.meta.resolve('fs') -> nodejs:fs

If the Node.js resolver has been hooked by a loader, the corresponding resolve function will be called in import.meta.resolve.

This API also makes the nodejs:fs internal builtin module scheme public, including the associated changes for this feature.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@guybedford guybedford added the blocked PRs that are blocked by other issues or PRs. label Dec 19, 2019
@guybedford
Copy link
Contributor Author

//cc @nodejs/modules-active-members

@guybedford
Copy link
Contributor Author

Discussion issue: nodejs/modules#458

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

I'm not a fan of exposing this in this way, at least not without reconciling with whatwg first. whatwg has expressed interest in a similar design, and I'd like us to match them up if/where possible.

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

@devsnek as mentioned in the issue comment, this PR is to start exactly those discussions, and is blocked on them.

@devsnek
Copy link
Member

devsnek commented Dec 19, 2019

@guybedford oh i see, I started writing my review before you posted that comment :) Glad to see we're on the same page 👍

@jkrems
Copy link
Contributor

jkrems commented Dec 19, 2019

await import.meta.resolve('./does-not-exist') -> Throws not found as the default Node.js resolver checks for existence.

Can we prevent that? It feels constraining when resolve has to know already if the URL can be loaded. E.g. we could return the resolution as if path extensions wouldn't be tried..? Basically I want to prevent that it becomes observable in node that .resolve('./foo') can actually check for foo's existence because the same code running elsewhere wouldn't realistically provide that info.

@ljharb
Copy link
Member

ljharb commented Dec 19, 2019

require.resolve already checks for existence and throws - while I'm not a fan of an exception there, the existence check is actually quite important for all the use cases of a resolver I'm aware of.

@guybedford
Copy link
Contributor Author

There would be nothing wrong with a custom resolver hook supporting resolving to files that don't exist and instead throw during getSource. That would be perfectly valid. But for the Node.js resolver against the file system it is currently very much a resolver feature.

@jkrems
Copy link
Contributor

jkrems commented Dec 19, 2019

If you're fine actually touching the file, it feels like you could just import it if you want to check that it's in fact reachable? E.g. throwing implies that if it doesn't throw, the file can be loaded. Which isn't necessarily true:

$ cat x.js
cat: x.js: Permission denied
$ node -e "require.resolve('./x.js')"
$ node -e "require('./x.js')"
internal/fs/utils.js:220
    throw err;
    ^

Error: EACCES: permission denied, open '/private/tmp/x.js'

It feels like it encourages people to assume that just because they could resolve it, there's some sort of guarantee that the returned string can be loaded.

@guybedford
Copy link
Contributor Author

If we want to move the existence check from the resolver to the instantiation phase, we can do that, but that should be considered a separate concern to this PR. Also someone would need to justify and create that PR before stability.

@jkrems
Copy link
Contributor

jkrems commented Dec 20, 2019

Right now we don't leak when exactly the exception happens to import, so it's just an implementation detail. I would consider leaking it a compat risk with the web since even if the web adds import.meta.resolve, I assume it won't be supporting existence checks. If we get signals from the web that they will include existence, then that changes this whole thing. :)

@ljharb
Copy link
Member

ljharb commented Dec 20, 2019

@jkrems it is definitely not an option to import() a file, and risk evaluating code, to check for existence - that's a big part of the point of a resolver, to know it will work before trying it.

@jkrems
Copy link
Contributor

jkrems commented Dec 20, 2019

Right, but that’s not actually possible to derive from the resolution result. As shown above, file permissions are enough for a resolved path not to be valid for import. And this doesn’t touch things like race conditions or loaders generating files on-the-fly (or hiding them even though they exist on disk).

What is a use case where you need to know that you could require something but where you can’t try to actually require it?

@ljharb
Copy link
Member

ljharb commented Dec 20, 2019

That there are more reasons why an import might not succeed (including file permissions, transitive imports failing, exceptions) doesn't change that "the specifier doesn't resolve to something" is an important bit to know prior to attempting to evaluate it. One example in particular, this bit helps you catch a programmer error on the importing side using eslint-plugin-import, which is used in eslint-config-airbnb.

@MylesBorins MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 23, 2019
@nodejs-github-bot
Copy link
Collaborator

@jkrems
Copy link
Contributor

jkrems commented Dec 26, 2019

One example in particular, this bit helps you catch a programmer error on the importing side using eslint-plugin-import, which is used in eslint-config-airbnb.

I know (and actively use) that plugin. But if that plugin wanted to start using import.meta.resolve, couldn't it just stat the resulting file to see if it's there? It could then even fail if it wasn't readable etc..

My primary problem with leaking it is quite simple: It means that import.meta.resolve is very unlikely to be portable for minimal gain. I'd be surprised if browser vendors would be willing to roll a resource existence / reachability check into a function to resolve URLs. This smells like URL.equals doing DNS queries in Java.*

(*) Yes, node's default resolver accesses the file system. But one can implement the ES resolver using a "small" in-memory data structure that doesn't require knowledge about every single file on disk (an import map). If file existence is required to implement resolution, import maps are no longer sufficient to model import URL resolution in node. To me that's a big property to give up on for a pretty fuzzy check of "could this URL actually be fetched".

@jkrems
Copy link
Contributor

jkrems commented Dec 28, 2019

For illustration of why I wouldn't want resolve to throw in general: Adding a step that tries to access the resource is easy. Undoing that exception is hard: jkrems@6cebcb2#diff-6dda3ef5f860111452d324058e12a738R162-R183

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

How would your alternative handle different kinds of exceptional cases, like in that snippet? How would the API return a success or multiple kinds of error values on the same channel, and why would that be an improvement?

@jkrems
Copy link
Contributor

jkrems commented Dec 28, 2019

I assume the only actual error case you'd likely be interested in would be that the file doesn't exist. And that can be checked by stating it. Without running the entire loader hooks chain (including source hooks and/or transforms), we won't be able to return any of this info longer term if loader hooks are registered. So there would be a resolve function that loads files from disks and compiles them. Which just seems wrong.

I'm happy to support an async isLoadableAsCode(url): boolean API in addition to this that would do all these checks. But I want to be able to resolve to a directory. Or to a text file I'll never import. So even ignoring the weird side effects to the implementation of .resolve, it feels like an awkward API to have opinions on what the returned URL is used for.

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

If i have to manually stat it, then “there’s a filesystem” isn’t abstracted from users of the resolver. One of the big benefits is to abstract that away. I shouldn’t need any fs methods at all to resolve a specifier, just the abstraction.

doc/api/esm.md Show resolved Hide resolved
@jkrems
Copy link
Contributor

jkrems commented Dec 29, 2019

If i have to manually stat it, then “there’s a filesystem” isn’t abstracted from users of the resolver. One of the big benefits is to abstract that away. I shouldn’t need any fs methods at all to resolve a specifier, just the abstraction.

I think I acknowledged that when I suggested that there could be a second function (isLoadableAsCode) that could do all the resource retrieval & content type testing required for more certainty. I just don't think that restricting import.meta.resolve to just this one case (only allow resolving to files that could likely be loaded via import as code) is worth it.

Especially because import.meta.resolve will unlikely be used by tools like eslint-plugin-import - because I assume for that plugin you wouldn't want to create an actual ES module with a faked URL inside of a node instance that has the proper content types supported you would need for the configuration of the plugin.

@devsnek
Copy link
Member

devsnek commented Dec 30, 2019

i think this argument may be a bit too isolated on the specific semantics of resolve. taking a step back, we have a pipeline somewhat like this:

import() => (specifier, referrer) => resolve() => url => getSource() => ...

The URL given to getSource() needs to be final. that is, getSource() understands how to load a found resource, but it does not understand how to find that resource.

Any behaviour needed to go from (specifier, referrer) to url should therefore be performed during resolve(). This may be as simple as new URL(specifier, referrer).toString() or it may be more complex, such as what our default resolver does.

The constraints of this model dictate that it must be assumed that resolve() can fail. For example, in our default resolver, missing permissions may preclude the read operation on a package.json, which means the algorithm doesn't know where to look for the entry file. The only reasonable behaviour is to bail.

@jkrems
Copy link
Contributor

jkrems commented Dec 30, 2019

For example, in our default resolver, missing permissions may preclude the read operation on a package.json, which means the algorithm doesn't know where to look for the entry file. The only reasonable behaviour is to bail.

I think that's fair. There's definitely exceptional circumstances where resolution cannot determine what the final url would be. I think throwing errors in that case would be the right thing to do.

@guybedford guybedford added pending and removed blocked PRs that are blocked by other issues or PRs. labels Jan 1, 2020
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

RSLGTM assuming this is

  • green CI
  • behind a flag

@MylesBorins
Copy link
Contributor

What else is needed for us to land this?

@jkrems
Copy link
Contributor

jkrems commented Jan 31, 2020

What else is needed for us to land this?

I think this is ready to land unless Guy protests.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Feb 3, 2020
PR-URL: #31032
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in 0f96dc2.

@guybedford guybedford closed this Feb 3, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
PR-URL: #31032
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
@ExE-Boss
Copy link
Contributor

The signatures of import.meta.resolve(…) and require.resolve(…) differ in terms of which arguments they accept: #31861.

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 3, 2020
PR-URL: nodejs#31032
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 3, 2020
Backport-PR-URL: #32610
PR-URL: #31032
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@codebytere codebytere mentioned this pull request Apr 4, 2020
@guybedford guybedford mentioned this pull request May 19, 2020
2 tasks
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants