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

Only apply /-prefix handling to special URL-like specifiers #227

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Oct 21, 2020

Closes #166.


Preview | Diff

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

The text and restriction seem like a good approach to me.

I just hope we don't restrict custom builtin schemes and alternative schemes though.

For example we might want to include safelisted schemes - whatwg/html#5482.


* |specifierKey| ends with U+002F (/),
* |normalizedSpecifier| [=string/starts with=] |specifierKey|, and
* either |asURL| is null, or |asURL| [=is special=]
Copy link
Collaborator

Choose a reason for hiding this comment

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

When would asURL be null here if the invariant is that it is already normalized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

asURL is null if normalizedSpecifier is something like "moment".

Copy link
Collaborator

@hiroshige-g hiroshige-g left a comment

Choose a reason for hiding this comment

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

LGTM

"file:text/": "https://example.com/lib/test-file/",
"ws:text/foo": "https://example.com/lib/test-ws/foo",
"ws:text/": "https://example.com/lib/test-ws/"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add HTTP, WSS (and HTTPS) schemes?
HTTPS scheme is tested elsewhere, but HTTP and WSS schemes are not tested at all.

@@ -47,6 +54,24 @@
"expectedResults": {
"/test": "https://example.com/lib/test2.mjs"
}
},
"Non-special vs. special schemes": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI this section currently needs blink internals APIs on WPT+Chromium, as data:, about: etc. schemes cannot be intercepted by service workers. I'll create a separate PR to move this section to e.g. url-specifiers-internal.json.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or you can split this section in this PR out to a separate file like:

{
  "importMap": {
    "imports": {
      "data:text/": "/lib/test-data/",
      "about:text/": "/lib/test-about/",
      "blob:text/": "/lib/test-blob/",
      "blah:text/": "/lib/test-blah/",
      "file:text/": "/lib/test-file/",
      "ftp:text/": "/lib/test-ftp/",
      "ws:text/": "/lib/test-ws/"
    }
  },
  "importMapBaseURL": "https://example.com/app/index.html",
  "baseURL": "https://example.com/js/app.mjs",
  "name": "URL-like specifiers",
  "tests": {
    "Non-special vs. special schemes": {
      "expectedResults": {
        "data:text/javascript,console.log('foo')": "data:text/javascript,console.log('foo')",
        "data:text/": "https://example.com/lib/test-data/",
        "about:text/foo": "about:text/foo",
        "about:text/": "https://example.com/lib/test-about/",
        "blob:text/foo": "blob:text/foo",
        "blob:text/": "https://example.com/lib/test-blob/",
        "blah:text/foo": "blah:text/foo",
        "blah:text/": "https://example.com/lib/test-blah/",
        "ftp:text/foo": "https://example.com/lib/test-ftp/foo",
        "ftp:text/": "https://example.com/lib/test-ftp/",
        "file:text/foo": "https://example.com/lib/test-file/foo",
        "file:text/": "https://example.com/lib/test-file/",
        "ws:text/foo": "https://example.com/lib/test-ws/foo",
        "ws:text/": "https://example.com/lib/test-ws/"
      }
    }
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I split it out into a separate file, and included HTTP, HTTPS, and WSS.

@hiroshige-g
Copy link
Collaborator

FYI Chromium CL: https://chromium-review.googlesource.com/c/chromium/src/+/2491594

@domenic
Copy link
Collaborator Author

domenic commented Oct 22, 2020

I just hope we don't restrict custom builtin schemes and alternative schemes though.

For example we might want to include safelisted schemes - whatwg/html#5482.

Protocol handlers only apply to navigation, not to resource fetching, so I don't think that would be the right layering.

@domenic domenic merged commit ad6f3bb into master Oct 22, 2020
@domenic domenic deleted the no-non-special-slashes branch October 22, 2020 15:39
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Reflecting
WICG/import-maps#227

Bug: 848607, WICG/import-maps#166
Change-Id: Ide80e105fc57dfa35a66051b241b699fa969fcec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2491594
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833322}
GitOrigin-RevId: 895e1cd024248d79a79cc0c2f5ec1fa99e8245b2
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

Successfully merging this pull request may close these issues.

data: / blob: URL mapping
3 participants