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

Handling of 301 redirects within script type module #613

Closed
guybedford opened this issue Feb 3, 2016 · 19 comments
Closed

Handling of 301 redirects within script type module #613

guybedford opened this issue Feb 3, 2016 · 19 comments

Comments

@guybedford
Copy link
Contributor

The implementation seems great, but I do have one small worry with the handling of redirects.

The problem

Correct me if I'm wrong, but to describe the use case here:

Say I have a server that acts like a registry, where https://registry.server.com/x.js redirects to https://some-cdn.com/x.js.

If I create a module tag that loads from https://registry.server.com/x.js it will store https://registry.server.com/x.js as the module registry record for x.

Now if https://some-cdn.com/y.js depends on import './x.js', that would resolve to https://some-cdn.com/x.js, and we'd end up with two registry entries for x, which would mean executing the code for x twice.

This kind of immediately defeats the point of supporting 301 redirects in the first place surely?

Naive Solution

When I was first writing code for these use cases, the way to handle this scenario seemed simple - we let the contents of https://registry.server.com/x.js be:

export * from 'https://some-cdn.com/x.js';

That then does the required thing by creating two entries in the registry, both reflecting the same module value without double execution of x.

Unfortunately though, in the spec, there is an exception that default is not included in export *. Thus if x had a default export we'd need to know that in advance and add an additional export {default} from 'https://some-cdn.com/x.js. This expectation of needing to know the shape of x ahead of time, breaks the isolation of concerns of creating a registry server that is independent of the package structure. Breaking package upgrades would need to be catered for in the registry, meaning it would need to be effectively a versioned registry service.

Please let me know if the problem and attempted solution are clear here? I'd be interested to hear thoughts, as it is small cases like this that can really break workflows. Supporting 301 redirects is already something that doesn't naturally fit into the module loader conceptual model (whatwg/loader#124 (comment)), and to do it for a use case that is effectively broken (as far as I can tell and please let me know if I'm wrong), seems unfortunate.

//cc @domenic

@annevk
Copy link
Member

annevk commented Feb 3, 2016

We did discuss this, and SharedWorker has a similar problem. We had not considered treating redirects as a network error. That would be possible, but also seems very surprising.

(Note that a registry probably does not want to use permanent redirects.)

@guybedford
Copy link
Contributor Author

Thanks for the quick response. I certainly understand it's a natural implementation path. I was wondering if there is no way to recreate the ideal naive case as described above - as in have a shell module at the original URL providing the same export bindings as the module at the redirected URL, representing the same module.

This way we would get:

import {x as x1} from 'https://registry.server.com/x.js';
import {x as x2} from 'https://some-cdn.com/x.js';

x1 === x2 // true

import * as X1 from 'https://registry.server.com/x.js';
import * as X2 from 'https://some-cdn.com/x.js';

X1 === X2 // false

So separate instances representing the same module bindings, while avoiding the duplicate execution.

Perhaps it is a somewhat convoluted approach, but it seems like it would cater to the use case.

@guybedford
Copy link
Contributor Author

(Corrected the code example to make sense)

@domenic
Copy link
Member

domenic commented Feb 3, 2016

Can't the server create these shell modules itself? I don't think we should muck with HTTP semantics here and reinterpret redirects as anything other than ... a redirect. Redirects are entirely transparent to the HTML level at this point.

@guybedford
Copy link
Contributor Author

Can't the server create these shell modules itself?

Of course it can, but that's passing the problem on. Let me stress again the module loader would also need to have a concept of responseURL now, which it doesn't currently. Consider also user normalization code within resolve that would need to be adapted to handle redirect cases. Basically there's a whole cascading series of problems that are opened up by this behaviour, and I'm not sure what benefit there is at all for the handling of redirects given these pain points.

I guess in a sense it is as good as throwing entirely as it becomes a use case that is never useful, but I was just wondering if there were ways to make it useful.

@annevk
Copy link
Member

annevk commented Feb 3, 2016

Note that you might want to have that concept anyway, since a service worker can also give you a response unrelated to the request.

@guybedford
Copy link
Contributor Author

@annevk that's a good point, but such a case would surely be handled just like its defined here, so this definition sets the precedent for the service worker scenario effectively?

@annevk
Copy link
Member

annevk commented Feb 3, 2016

If we decided here that we would not follow redirects and treat that as an error, the service worker problem would not automatically disappear. There is a bit of debate though whether the browser always uses the response URL to resolve URLs or the last request URL. What we do would depend on that as well.

@domenic
Copy link
Member

domenic commented Feb 3, 2016

I don't think the spec is going to change here. It is currently consistent with the rest of the web platform, in a good way that enables many normal use cases (such as moving a site to https).

There is nowhere else in the platform that rejects 3xx responses. And there is definitely nowhere where we intercept a 3xx response, and instead say "oh server, you didn't really mean to redirect me. Let me, the browser, override your intent and instead replace the response with a 200 ok containing some new content I've synthesized that I think will do something similar to what you really meant." If the server wants to create proxy modules re-exporting from another module, it should do so---we shouldn't bake that kind of ad-hoc solution to a specific use case into the platform.

The module loader as specced in HTML already has a concept of response URL. Any module loader which does not have that is just deficient.

@annevk
Copy link
Member

annevk commented Feb 3, 2016

(A service worker is always fetched without following redirects. Preflighted CORS fetches don't either, though that we are fixing.)

@guybedford
Copy link
Contributor Author

@annevk thanks, just to clarify as I'm not that up to date on the service worker interaction, so does that mean using a service worker in the example above would effectively cause different resolution semantics for relative imports?

@domenic the intent of a 3xx is to redirect. The current implementation does not satisfy this intent, as if it were a true redirect, it would not result in duplicate execution, it would result in a module registry entry at the redirected location. I'm not asking to change the platform semantics, I'm just asking if there's ways to make it behave as a true redirect, and if not why it is implemented at all, as it seems like a footgun.

@annevk
Copy link
Member

annevk commented Feb 3, 2016

@guybedford not by default (although even that is something @jakearchibald is considering changing: https://jakearchibald.com/2016/service-workers-and-base-uris/), but it would be possible.

@guybedford
Copy link
Contributor Author

I'm certainly not pushing for implementation changes, I just felt this needed to be brought up somewhere, and for myself as well to confirm the consequences. I guess this is a little more pressing since export * can't be used for these redirections due to it skipping the default, which is unfortunate.

A custom resolve hook effectively then remains the best way to handle redirection from a module perspective as far as I can tell.

@annevk
Copy link
Member

annevk commented Feb 3, 2016

It seems this issue mostly has consequences if the redirect affects the base URL. E.g., if everything points to /x and /x redirects to /y, things will still work. If you then want to update pointers, you best update them all at once to avoid duplication. It's a little screwy, but not being able to change URLs is pretty bad too.

@guybedford
Copy link
Contributor Author

@annevk say a dependency of y tries to load /y later on, then you would still get duplicate execution though? Consider also if I load /x which redirects to /x/main.js, then a later import to /x/main.js would also causes duplicate execution. This stops for example the possibility of a simple main mapping system support for npm packages working through redirects.

@domenic
Copy link
Member

domenic commented Feb 3, 2016

@domenic the intent of a 3xx is to redirect. The current implementation does not satisfy this intent, as if it were a true redirect, it would not result in duplicate execution, it would result in a module registry entry at the redirected location.

I disagree with this. People are referring to the module by two different names---who can say what their intent was? Should we also normalize import './foo.js#x' and import './foo.js#y' to the same? What about ?x and ?y? What about foo.js and bar.js where those two files have the same contents? What about server-side symlinks?

I don't think a redirect implies that they should be the same registry entry (or a proxy module delegating to another registry entry), in any sense. It just implies the semantics of a HTTP redirect. One of the important parts of those semantics is that request URL and response URL differ, which can definitely be done intentionally.

I'm not asking to change the platform semantics, I'm just asking if there's ways to make it behave as a true redirect, and if not why it is implemented at all, as it seems like a footgun.

I disagree that it's a footgun. Referring to a module by two different URLs seems like a pretty good reason to execute it twice.

@guybedford
Copy link
Contributor Author

@domenic to clarify the redirect intent, lets consider a good old-fashioned redirect scenario - https://modules.com is a module server. As a user of this module server, on my own custom website I write my modules like:

<script type="module">
import $ from 'https://modules.com/jquery.js';
import _ from 'https://modules.com/lodash.js';
</script>

For argument's sake, lets imagine the jQuery of the future depends on lodash, so it contains import _ from './lodash.js' within its source.

Now https://modules.com decides to move its domain to the much better sounding name, https://awesome-modules.com.

They set up 301 redirects for all the modules, but now my website which uses the old URLs to https://modules.com is executing lodash twice. Thus modules.com cannot use 301's to effectively redirect modules to the new domain.

In this very traditional sense of a redirect, this is what I mean by it not fulfilling the intent.

I wanted to suggest the shell module approach which does satisfy the intent, but I completely understand if there is no interest or it is unsuitable here. But please be aware that work is being made for the loader spec to support this, without a clear benefit, which could be avoided as well as satisfying the use case through the shell module approach mentioned.

@domenic
Copy link
Member

domenic commented Feb 3, 2016

But please be aware that work is being made for the loader spec to support this

If the loader spec introduces shells, I don't think it will be acceptable to implementers. You can't just break HTTP semantics like that.

@guybedford
Copy link
Contributor Author

Sure, I understand. Thanks for hearing me out on this.

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

No branches or pull requests

3 participants