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

import.meta.resolve implementation #7296

Closed
guybedford opened this issue Aug 31, 2020 · 26 comments · Fixed by #15074
Closed

import.meta.resolve implementation #7296

guybedford opened this issue Aug 31, 2020 · 26 comments · Fixed by #15074
Labels
deno_core Changes in "deno_core" crate are needed suggestion suggestions for new features (yet to be agreed)

Comments

@guybedford
Copy link
Contributor

Node.js has been shipping an import.meta.resolve implementation for some time behind the --experimental-import-meta-resolve flag described briefly here - https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_no_require_resolve.

In Deno such a function could resolve the local import map when set including applying scope resolution based on the current module context.

Is there interest from Deno in implementing this API? I don't think it would be sensible for Node.js to unflag this feature without getting wider support, but Deno is likely the only other environment that might benefit from this. Feedback would help us to consider the unflagging process.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 1, 2020

We shouldn't implement this unless browsers do. Also I don't think it's possible to make this work with bundling... not in a way that's consistent with our handling of other import.meta fields. It would have to know the full URL at bundle-time for import map resolution. Would bundled code look up the import map at runtime? Edit: Yes, no problem with that really.

@guybedford
Copy link
Contributor Author

The model for require.resolve in bundles we use in ncc is to reemit the asset or folder that was resolved based on static analysis of the expression flow and then to replace a direct asset reference into that expression.

The same model can apply to import.meta.resolve nicely because it is exactly a statically analyzable construct.

Browsers have no driving need to implement this, so if we (Node.js and Deno) wait, we may wait years. I'm happy to leave this open to track further though.

@lucacasonato
Copy link
Member

This looks like something that can be fully implemented in userland with import.meta.url in Deno.

We don't have node resolution, so I guess the implementation would more or less look like this: import.meta.resolve = (path, base = import.meta.url) => new URL(path, base).toString();.

It also does not really make sense for this to return a Promise<string> in Deno because we can resolve this synchronously without IO ops, just like the browser. But I guess Node would require this to be async...

@bartlomieju
Copy link
Member

That method should probably take into account currently loaded import map, so it would actually be an op.

@lucacasonato
Copy link
Member

Ah right - import maps... 🤦

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 1, 2020

Yep, Deno.resolveModule(fullyQualifiedUrl) since it's a host environment thing. Or more likely wait for the web to have something similar. And if they implement that in tc39 import.meta as opposed to a global web API, we would too.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 1, 2020

TC39 doesn't deal with things like module resolution. import.meta was specifically designed to deal with the stuff TC39 doesn't. It is WhatWG that has attempted to move forward on things like this, providing import maps in the first place.

@guybedford is there no interest in WhatWG that you are aware of? I know module loading and resolution is never a straightforward topic! 😉

@kitsonk
Copy link
Contributor

kitsonk commented Sep 1, 2020

(Also for the record, import.meta was adopted by TC39 for some of these very specific reasons: https://github.com/tc39/proposal-import-meta. It is a "catch all" for runtime specific implementations. So alignment between Deno and Node.js makes a lot more sense than throwing it into a truly unique namespace like Deno IMO)

@guybedford
Copy link
Contributor Author

The benefit of import.meta.resolve is having a standard resolve function which build tools can statically analyze regardless of platform to eg perform asset relocation (https://github.com/vercel/webpack-asset-relocator-loader).

If Node.js and Deno have their own custom functions and encourage that, those patterns get engrained in usage and users have code that does conditional checks and calls these different functions - the build tooling to deal with bundling while retaining asset references becomes very specific to each platform then or needs lots of custom work.

The custom usage for a library then looks something like:

function crossPlatformResolve (specifier) {
  if (typeof Deno !== 'undefined') {
    return Deno.resolve(specifier, import.meta.url);
  } else if (typeof process !== 'undefined' && process.platform.node) {
    // nodejs has yet to specify such a custom resolver too
    return require('module').nodeResolve(specifier, import.meta.url);
  } else {
    // etc for all other platforms
    throw new Error('no resolver found');
  }
}

const template = await (await fetch(crossPlatformResolve('pkg/template.html')).text();

when it could just have been:

const template = await (await fetch(import.meta.resolve('pkg/template.html')).text();

which systems like asset relocators can then statically replace with:

const template =  await (await fetch(import.meta.url + '../path-to-inlined-template.html'));

as a pattern for bundling asset references.

The alternative of custom function patterns without statically analyzable assets being easy becomes the path the longer we wait, and it's fine, but we can just do much better if we can move now.

As mentioned, I feel that environments like Deno and Node.js have the driving use cases here, the browser doesn't necessarily. Waiting on WhatWG means giving up agency, as neither Deno nor Node.js have any voice at WhatWG.

There was a WhatWG PR for this in whatwg/html#5572, the main contentions there were whether it should be sync or async, and whether there should be a second argument or not.

To give you the gist of the current situation:

  1. If both Node.js and Deno were to decide that a sync import.meta.resolve function without a second argument is fine, that would get us alignment with Domenic and a potential path to move forward immediately
  2. Alternatively, both platforms can choose to collaborate on driving work here with the API that suits these platforms best.
  3. Avoid scary decisions entirely and do nothing.

I understand completely (3) is the easy status quo though :)

@timreichen
Copy link
Contributor

timreichen commented Sep 3, 2020

@guybedford I would vote against implementing a non-standard like that.
Agree with @nayeemrmn that if this was implemented it would be better to wait or implement it as a function in std/node since it is kinda node specific.

@lucacasonato
Copy link
Member

@timreichen I didn't say this should be implemented in the Deno namespace. And also I disagree that this is node specific. This problem is something that applies to Deno too. Only the sync vs async implementation is where we diverge from Dominic's proposal.

@timreichen
Copy link
Contributor

@lucacasonato sorry read that wrong. Corrected.

@nayeemrmn
Copy link
Collaborator

Re sync vs async, the permissions API would be a precedent for us going async to match the standard even though the underlying calls are sync. I don't see what reason anyone would have to ever "expect" a sync API without concerning themselves with the implementation.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 3, 2020

Well, we in theory want to be able to resolve async, especially for remote modules. This is where the semantics differ from Node.js in that would we return the initial URL for a module, or the final redirected URL for a module?

All this being said, @bartlomieju and I are embarking on some major refactors of how we do dependency analysis determining what processing or not needs to be done to modules. It feels like this is something we might want to consider after the dust settles on that.

@guybedford
Copy link
Contributor Author

Thanks for the feedback here, it does sound like those refactorings / resolver concerns in Deno need to be fleshed out more in order to determine what is best for Deno here further with regards to APIs here.

It would be great to work towards alignment between Node.js and Deno when those directions are ready, and I will continue to reiterate that specifications are secondary to implementations, and in this space Node.js and Deno have the interest end engagement of the implementation / ecosystem conventions first and foremost. There is certainly risk diverging from browsers yes, but if we were to eg jointly propose such browser specs as part of the implementation / unflagging process I'm sure it would be well received. I'd be more than happy to collaborate on specification work here as well.

@bartlomieju bartlomieju added deno_core Changes in "deno_core" crate are needed suggestion suggestions for new features (yet to be agreed) labels Nov 18, 2020
@bartlomieju
Copy link
Member

I don't think there are any more blockers to this feature.

Our internal infrastructure has synchronous APIs for resolving module specifiers so I'm not sure if we need to to be async. @kitsonk what's the use case for remote modules you mentioned in #7296 (comment) ?

@bartlomieju
Copy link
Member

So Node has import.meta.resolve() available behind a flag (https://nodejs.org/api/esm.html#esm_import_meta_resolve_specifier_parent) and it's an async function.

Again, from my point of view there are no blockers for this feature and we could implement it easily.

@guybedford are there are signals from committees/browser vendors about interest in the API?

@guybedford
Copy link
Contributor Author

@bartlomieju browsers are definitely interested, although I believe still for a sync version not an async version. What's your opinion on that matter? Node.js loaders are fully async but in theory could be syncified using separate processes. At a low level though resolve has always felt like an async operation when considering CDN lookups to me. But I'm open to changing my mind.

@bartlomieju
Copy link
Member

@bartlomieju browsers are definitely interested, although I believe still for a sync version not an async version. What's your opinion on that matter? Node.js loaders are fully async but in theory could be syncified using separate processes. At a low level though resolve has always felt like an async operation when considering CDN lookups to me. But I'm open to changing my mind.

I do tend to agree with you. Currently resolve() is sync in Deno as well, but I can see it might change in the future. I believe going with async is more future-proof than having to add resolveAsync() API in a few years.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 17, 2021

Currently resolve() is sync in Deno as well

For a resolved module graph. Try to resolve something outside that module graph, then it would become async again.

silly phone

Unless we accept that the resolve simply "guesses" at what the target is, not accounting for redirects, etc.

@kitsonk kitsonk closed this as completed Sep 17, 2021
@kitsonk kitsonk reopened this Sep 17, 2021
@bartlomieju
Copy link
Member

Currently resolve() is sync in Deno as well

For a resolved module graph. Try to resolve something outside that module graph, then it would become async again.

silly phone

Unless we accept that the resolve simply "guesses" at what the target is, not accounting for redirects, etc.

@kitsonk I thought this utility is just for resolving URLs? Unless there are some DNS lookups done this is purely computational utility? At least that is the signature that we have in built-in ModuleLoader.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 17, 2021

Yeah, but the module loader calls the module graph and the module graph has all the dependencies resolved including the import map. I will take a look at it deeper to see if we change our response based on what is on the graph, or if we wait until the load. I am just saying there is a risk that we might not have all the information to hand to give a definitive response.

@bartlomieju
Copy link
Member

Yeah, but the module loader calls the module graph and the module graph has all the dependencies resolved including the import map. I will take a look at it deeper to see if we change our response based on what is on the graph, or if we wait until the load. I am just saying there is a risk that we might not have all the information to hand to give a definitive response.

I see what you mean and that's okay. Anyway I'm in favor of API being asynchronous like it is in Node.

@nayeemrmn
Copy link
Collaborator

@kitsonk Is that assuming import.meta.resolve() may have to follow redirects? We already know it doesn't or it would have to be async on the web as well. Redirects generally don't need to resolved across. We need a way to resolve URLs across import maps because other bindings like fetch() don't refer to them by default, and sometimes you'd want them to. Whereas an unredirected URL is externally led to the same resource regardless of the binding.

It seems import.meta.resolve() just joins URLs and then passes them through a preloaded import map. Fundamentally Deno can ship a sync API as self-assuredly as browsers can, the relevant argument I can find for making it async (paging large import maps 😕) also applies to Deno as much as it does to browsers. So as always we should just do what browsers do or at least what we guess they are going to do (my bet is on sync) ¯\_(ツ)_/¯

@guybedford
Copy link
Contributor Author

Discussing with @domenic further on this it does sound like it will be unlikely that Chromium would implement async here, and that a lot of the internal mechanics are built around sync resolve.

Node.js will have a hard time going sync as it involves syncifying the loader pipeline, but it may be worth Deno reconsidering going sync here to try to aim for browser alignment.

That would then mean treating any async resolution work or lazy import maps extensions as part of separate mechanisms that don't block resolve, and could have their own lifecycle events.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 22, 2021

That would then mean treating any async resolution work or lazy import maps extensions as part of separate mechanisms that don't block resolve, and could have their own lifecycle events.

Any of that would be on the Rust side, which we can always "op" sync if we need to, even if it is back by async operations, just means the event loop would be paused. We used to do it with embedded tsc as it requires a sync resolve. The only thing that I can think of is trying to resolve a remote import that wasn't statically analysable as part of the graph at startup, but then again, we could "guess" at the resolved specifier and just fail on a load. We would resolve redirects anyways on load if a specifier was provided that got redirected.

Currently, when you try to resolve/load something outside of the current statically analysable graph (like a non-analyzable dynamic import or a web worker script) you end up building another static dependency graph which gets loaded, checked and emitted before returning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deno_core Changes in "deno_core" crate are needed suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants