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.url needs its fragment censored #3622

Closed
domenic opened this issue Apr 10, 2018 · 10 comments
Closed

import.meta.url needs its fragment censored #3622

domenic opened this issue Apr 10, 2018 · 10 comments

Comments

@domenic
Copy link
Member

@domenic domenic commented Apr 10, 2018

Edit: to those coming to this thread fresh, the OP may be confusing. The conclusion of this thread was that the spec was good as-is, but we needed to add tests because browsers were not consistent. The tests were added in web-platform-tests/wpt#10717. Keep reading for the gory details.

Because it exposes a response URL for the script, the fragment should be censored, per Fetch's definition of response URL:

A response URL is a URL for which implementations need not store the fragment as it is never exposed. When serialized, the exclude fragment flag is set, meaning implementations can store the fragment nonetheless.

However, it appears that specs need to carefully censor this themselves, e.g. like Fetch does in the response.url getter.

HTML was not careful about this, and @bmeck has reported that this caused interop problems where Safari followed the HTML spec verbatim (and so included the fragment) whereas Chrome used their usual response-URL infrastructure (and thus didn't have a fragment around).

Almost certainly the right thing to do here is censor the fragment and add a WPT ensuring that it stays that way.

/cc @annevk @whatwg/modules

@bmeck
Copy link

@bmeck bmeck commented Apr 10, 2018

If this only is the response URL I would request exposing a way to get the full URL. What is the reason to not include the fragment? It seems weird that we execute multiple times but are unable to determine the fragment and therefore cache entry to bust that the execution is tied to.

@domenic
Copy link
Member Author

@domenic domenic commented Apr 10, 2018

It's not really a choice between exposing the fragment and not. It's between exposing the response URL (never includes a fragment) and exposing the request URL (may include a fragment). The response URL is generally the useful one, since e.g. if you do fetches relative to the request URL, you will probably get the wrong result.

For example, consider /a.mjs which redirects to /resources/b.mjs. We include it like so:

<!-- https://example.com/index.html -->
<script type="module" src="a.mjs">

Inside the redirected-to https://example.com/resources/b.mjs, we have

fetch(new URL("foo.json", import.meta.url));

Currently, this fetches https://example.com/resources/foo.json, as expected. If import.meta.url were the request URL, this would try to find https://example.com/foo.json, which would not be correct.

@bmeck
Copy link

@bmeck bmeck commented Apr 10, 2018

@domenic my understanding was that import.meta.url was intended to identify the executing Module. It seems that is not the case?

@domenic
Copy link
Member Author

@domenic domenic commented Apr 10, 2018

No, it's intended to give you a URL relative to which you can find other resources. It's not meant to serve as a kind of identifier into the (invisible-to-developers) module map.

@bmeck
Copy link

@bmeck bmeck commented Apr 10, 2018

Interesting, not what I would hope for. Might need to come back to this since I think it would be a big enough thing for me to think about requesting removing from Node. Need to think.

@annevk
Copy link
Member

@annevk annevk commented Apr 11, 2018

I think per whatwg/fetch#505 we want to expose the fragment here (and basically stop hiding it everywhere).

@domenic
Copy link
Member Author

@domenic domenic commented Apr 11, 2018

Oh, wow, I forgot about that thread. Browsers even magically copy the fragment across redirects!? Amazing.

@guybedford
Copy link

@guybedford guybedford commented Apr 12, 2018

I take it this can be closed then?

@annevk
Copy link
Member

@annevk annevk commented Apr 12, 2018

I think we probably still want to make sure it's tested.

domenic added a commit to web-platform-tests/wpt that referenced this issue Apr 30, 2018
domenic added a commit to web-platform-tests/wpt that referenced this issue May 10, 2018
@domenic
Copy link
Member Author

@domenic domenic commented May 10, 2018

Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=841831

Other browsers don't seem to have implemented import.meta yet :(

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 18, 2018
…s, a=testonly

Automatic update from web-platform-testsTest import.meta.url preserving fragments

Closes whatwg/html#3622.

--

wpt-commits: 38a9456c9ff3d2e666cf404415e472e11fffd43d
wpt-pr: 10717
sole pushed a commit to sole/gecko that referenced this issue May 21, 2018
…s, a=testonly

Automatic update from web-platform-testsTest import.meta.url preserving fragments

Closes whatwg/html#3622.

--

wpt-commits: 38a9456c9ff3d2e666cf404415e472e11fffd43d
wpt-pr: 10717
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…s, a=testonly

Automatic update from web-platform-testsTest import.meta.url preserving fragments

Closes whatwg/html#3622.

--

wpt-commits: 38a9456c9ff3d2e666cf404415e472e11fffd43d
wpt-pr: 10717

UltraBlame original commit: f424009146ecf744fea3f99db4943d7ae2837d74
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…s, a=testonly

Automatic update from web-platform-testsTest import.meta.url preserving fragments

Closes whatwg/html#3622.

--

wpt-commits: 38a9456c9ff3d2e666cf404415e472e11fffd43d
wpt-pr: 10717

UltraBlame original commit: f424009146ecf744fea3f99db4943d7ae2837d74
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…s, a=testonly

Automatic update from web-platform-testsTest import.meta.url preserving fragments

Closes whatwg/html#3622.

--

wpt-commits: 38a9456c9ff3d2e666cf404415e472e11fffd43d
wpt-pr: 10717

UltraBlame original commit: f424009146ecf744fea3f99db4943d7ae2837d74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants