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

Add support for redirects #32

Closed
kriskowal opened this issue Oct 28, 2020 · 5 comments
Closed

Add support for redirects #32

kriskowal opened this issue Oct 28, 2020 · 5 comments

Comments

@kriskowal
Copy link
Member

In Node.js, a module specifier ./utility may be fulfilled by a module with the full specifier ./utility/index.js instead. To implement this, the loadHook must return not just the StaticModuleRecord, but also the actual full specifier of ./utility/index.js because that is the referrer specifier for any import specifier in that module.

On the web a similar facility is necessary for redirects. In Node.js the same facility is also necessary for canonicalized filesystem paths (realpath).

To support these cases, we can overload the return value of loadHook:

ModuleRecord | { record: ModuleRecord, specifier: FullSpecifier }

Are there objections or alternatives to this change?

(I specified ModuleRecord above as potentially either StaticModuleRecord | ThirdPartyModuleInterface, which is orthogonal)

@guybedford
Copy link
Collaborator

guybedford commented Oct 28, 2020

I feel quite strongly the concept of a referrer specifier is broken and should be avoided if possible.

On the web, module redirects suffer from the following double instance issue:

  1. Consider /pkg redirecting to /pkg/core.js
  2. Consider /pkg/feature.js importing ./core.js locally
  3. If I import /pkg and then import /pkg/feature.js this will result in execution of /pkg and /pkg/core.js as two separate double instantiations of the the core module in the loader registry. Leading to both side effect and instancing issues.

As a result I would rather not see this mistake extended to new functionality like compartments.

@devsnek
Copy link
Member

devsnek commented Oct 28, 2020

this sounds better solved by #18

@kriskowal
Copy link
Member Author

I do not think there’s a real world where all import specifiers are fully qualified and refer exactly to the executing source. It may be possible to effect that reality on some hosts and virtual hosts, but definitely not on all of them. My hope is that a loader API is big enough to allow for at least some existing code targeting an existing host be possible to carry forward.

@kriskowal
Copy link
Member Author

By way of an update, the current draft solves this problem with “module descriptors”, which are a union of various kinds. The relevant ones allow the unified loadHook or the modules option (née moduleMap separate argument) to provide an alternate referrerSpecifier such that importSpecifiers can be resolved correctly. One form of redirect looks like { instance: fullSpecifier }, another like { record: fullSpecifier }.

In #52, I entertain a conversation to add back { record: [Synthetic]StaticModuleRecord, specifier: fullSpecifier } to avoid a rebounce off the loadHook trampoline, but it’s not the most idealistic solution. It is however what we implemented in the SES shim and works to emulate Node.js’s behavior for following symbolic links.

@kriskowal
Copy link
Member Author

I’ve closed the issue but discussion remains open for any aspect of the proposal. Please feel free to open, or open an alternative issue to tear the above redirect mechanisms out of the proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants