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: unflag import.meta.resolve #33464

Closed

Conversation

guybedford
Copy link
Contributor

Now that TLA has landed, the workflows around this are really quite nice.

For example, to load the package.json file from an external package one can write:

import { promises as fs } from 'fs';
const pkgUrl = await import.meta.resolve('pkg/');
const pjson = await fs.readFile(new URL('package.json', pkgUrl));

Note: this is in no way meant to be a full solution to the other issue discussion, where @ljharb has suggested some directions forward.

Other examples were included in the original PR - #31032.

I do think unflagging is important as this is a critical feature for ESM to ensure it has the require.resolve use cases supported.

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. labels May 19, 2020
@ljharb
Copy link
Member

ljharb commented May 19, 2020

I'm concerned about import.meta.resolve('pkg/') specifically - I don't think any path should be resolveable unless it's importable, just like in CJS.

cc @nodejs/modules-active-members

@guybedford guybedford added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label May 19, 2020
@guybedford
Copy link
Contributor Author

@ljharb it is not importable because the difference between import.meta.resolve and import() is that the ERR_UNSUPPORTED_DIR_IMPORT resolution error is caught and allowed by returning its url error data property.

@guybedford
Copy link
Contributor Author

Other than that though, the resolvers are equivalent.

@guybedford
Copy link
Contributor Author

Other useful use cases for this include resolving "exports": { "./assets/": "./path/to/assets/" } style exports paths where it wouldn't otherwise be clear how to determine the subpathing.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

I think that if it's useful to be able to resolve non-importable paths (ie is, in both CJS and ESM, it's just not possible right now in CJS), then we need to pursue an API for both CJS and ESM to do so. The point of import.meta.resolve imo is to be the ESM version of require.resolve, and thus should function like that.

@devsnek
Copy link
Member

devsnek commented May 19, 2020

this was originally flagged because we wanted to work with the web to have some isomorphism here, is that still a goal?

@guybedford
Copy link
Contributor Author

@ljharb the benefit of a singular API is static analyzability - import.meta.resolve and import.meta.url are primary analysis targets for build tools. If we introduce new APIs like import { getPackageRoot } from 'module' there is less chance of success in getting build tool analysis buy in.

I say this having done a lot of these types of analyses in node-file-trace and ncc for Vercel.

ESM is more statically analyzable than CJS - we didn't have to restrict ourselves in the development of ESM to making it match CommonJS in terms of analyzability. You seem to be applying the same sort of argument in disallowing features which improve analyzability.

I am not against a import { getPackageBase } from 'module' API - and I specifically indicated this in the PR description.

Ideally we can avoid seeing problems as mutually exclusive and appreciate that there are different benefits to different approaches. I don't think we should restrict the import.meta.resolve API to only require.resolve features though.

@guybedford
Copy link
Contributor Author

@devsnek if we wait for the web we will grow old in the mean time, and ship ESM without full features.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

@guybedford i'm not opposed to the ESM version of it being chained off of import.meta, but "resolve" is for importing. You can't resolve a path you can't require/import. I do think we should restrict ourselves to feature sets - anything that's possible to do in both, must be done in both, and must not be done in one. CJS and ESM are both first-class module systems in node for the foreseeable future.

@devsnek
Copy link
Member

devsnek commented May 19, 2020

The resolver's job is to turn a specifier and a referrer into an absolute url. knowing whether that url can actually be loaded is separate, and requires a full import lifecycle.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

@devsnek while i don’t think that’s true in node, where all importable URLs are synchronously resolvable, the ESM API should then be async. The entire point of a resolve api is so you can load the result.

@devsnek
Copy link
Member

devsnek commented May 19, 2020

no the point is to pass the resolved url to the source loader, which is the only part of node that understands whether or not the url can be loaded

@jkrems
Copy link
Contributor

jkrems commented May 19, 2020

I agree with @devsnek here. It's already not fully true that a string generated from require.resolve can be required in the face of disk operations (we don't pre-load the contents to ensure require succeeds). In ESM (import.meta.resolve) and in the presence of loaders, we can't even make an educated guess because we would either have to run the entire loader pipeline (including loading the source and transforming it) or we would throw for things that actually load fine if imported.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

Given the description, it looks like one of two things are true:

  1. Resolving pkg/ doesn't honor ./ in exports, e.g. for people who want to do things like {"./": "./lib/"}.
  2. This isn't a reliable way to get the root package.json because it may accidentally resolve to ./lib/packag.json.

I'm still in support of unflagging this but I currently assume that (2) is the case. I would have concerns if this is (1).

@ljharb
Copy link
Member

ljharb commented May 19, 2020

In the absence of loaders (the default and common case), and assuming disk operations succeed (also the common case), require.resolve is requireable, and i expect (and i maintain, the common expectation will be) import.meta.resolve to be importable.

@jkrems
Copy link
Contributor

jkrems commented May 19, 2020

the common expectation will be import.meta.resolve to be importable.

That assumes the HTTP-based imports will never be a common thing. To me that's too big a bet to enshrine in our APIs. Especially given that it absolutely is a common thing in browsers and browsers are by far the most popular JS [module] runtime. I'm not saying I'm 100% sure that they will become common, just that I wouldn't want to design our APIs based on the assumption that they won't.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

I don’t think it will ever be a good idea nor a possible thing, but that still doesn’t mean this api should ever resolve a path that is known to be impossible to import.

@devsnek
Copy link
Member

devsnek commented May 19, 2020

@ljharb it sounds like you're asking that the default resolver fail if it knows the resolved url won't be loadable?

@ljharb
Copy link
Member

ljharb commented May 19, 2020

@devsnek yes, just like require.resolve does.

@devsnek
Copy link
Member

devsnek commented May 19, 2020

@ljharb that's not a change that would be made to import.meta.resolve then, it would be made to the default resolver.

@ljharb
Copy link
Member

ljharb commented May 19, 2020

@devsnek ok - why would the default ESM resolver allow pkg/ in the first place if the package has "exports", and didn't specify "./": in it?

(also, the point of import.meta.resolve imo is that it is the default resolver, just like require.resolve is for CJS)

@devsnek
Copy link
Member

devsnek commented May 19, 2020

@ljharb import.meta.resolve returns whatever the resolver chain returns, which includes loaders. I can't answer the question about pkg/ because idk how package exports work.

@MylesBorins
Copy link
Contributor

Has there been any signal from upstream that a standardized version of this is ready to land in browsers? I'm uncomfortable unflagging this until that point.

While we do have TLA, it is still behind a flag... this to me seems premature

@bmeck
Copy link
Member

bmeck commented May 20, 2020

I believe the closes thing is Compartments have a resolve hook. It does not currently allow you to reflect on your current context, but that was talked about in the last 2 meetings as a potential fix for a problem with polyfills.

@jasnell
Copy link
Member

jasnell commented May 20, 2020

Given that import.meta is intended to contain implementation defined metadata properties, I do not think we really have to wait on whether browsers get this mechanism or not. As I understand it, import.meta did intentionally design to allow hosts to add things that might be specific to that host.

@guybedford
Copy link
Contributor Author

As just discussed in the modules meeting, there was resistance to implementing this without having exact implementation equality with require.resolve and existing standards work for import.meta.resolve.

@guybedford guybedford closed this May 20, 2020
@GeoffreyBooth
Copy link
Member

I think there is consensus that we can ship an equivalent:

import { resolveURL } from 'module'; // <---- this

import { promises as fs } from 'fs';
const pkgUrl = await resolveURL(import.meta, 'pkg/');
const pjson = await fs.readFile(new URL('package.json', pkgUrl));

And potentially add it to import.meta later on when there's more standardization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. modules-agenda Issues and PRs to discuss during the meetings of the Modules team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants