WICG / import-maps

How to control the behavior of JavaScript imports
https://html.spec.whatwg.org/multipage/webappapis.html#import-maps
Other
2.68k stars 69 forks source link

Clarification: #176 and invalid URLs #184

Closed hiroshige-g closed 4 years ago

hiroshige-g commented 4 years ago

176 changed the behavior around invalid URLs. i.e.

{"imports": { "https://example.com/foo": "https://:invalid:url" }}

is equal to

What was the reason behind this change?

Does this mean that, after #176, when we re-enable fallback/built-in modules support again:

  1. {"imports": { "https://example.com/foo": "https://:invalid:url" }} is equal to {"imports": {}} as a direct result of the change #176,
  2. {"imports": { "https://example.com/foo": ["https://:invalid:url"] }} is equal to {"imports": {}} for keeping equivalence between "https://:invalid:url" and ["https://:invalid:url"] that held before #176,
  3. {"imports": { "https://example.com/foo": [] }} is equal to {"imports": {}},
  4. {"imports": { "https://example.com/foo": null }} is different from {"imports": { "https://example.com/foo": [] }}

?

Context: Chromium will keep fallback implementation behind built-in modules flag, even after #176, and thus I'd like to clarify the background/implication of #176 to the fallback mechanism. I'm neutral about the new/old behaviors.

domenic commented 4 years ago

This was not explicitly intentional; it likely fell out of some refactoring or was the most natural way of coding things.

If we add fallback support back, I would like null and [] to remain equivalent.

Since the most natural behavior for ["https://:invalid:url"] is to collapse it to [], then I think it should be in that equivalence class too.

Then, of course, "https://:invalid:url" and ["https://:invalid:url"] should also be equivalent, so it seems we are forced to have them all in the same equivalence class.

So the question is what should mapping to null do: cause an error, or be ignored. Probably it should cause an error, since that is more useful?

In that case we should probably move back to the old behavior. This means complicating the model somewhat by changing a specifier map to be strings -> URL or null instead of just string -> URL.

hiroshige-g commented 4 years ago

the question is what should mapping to null do

+1.

I'm a little leaning toward ignoring mapping to null (i.e. allow fallbacks to other candidates) because other parts of import maps also work in fallback-based fashion:

Also, even when a mapping to null causes an error, the resolution can be still successful, e.g. when the specifier is URL-like (fallback to Step 11) or there are less-specific scopes. (Still we could make the resolution failed by changing the spec though)

Anyway, this is a very slight preference. In the short-term, this is (slightly) blocking WPT test migration, so I'll change the implementation to "ignore" null mappings (if there're no additional inputs) and possibly reconsider this issue later again.

guybedford commented 4 years ago

Without standard modules like @notfound or @empty, having null as a way to block an import from resolving can be quite useful. For example, to disable builtin modules.

Previously I was under the impression that fallbacks like [] throw since there is no resolution found? And that null was just an alias of that?

hiroshige-g commented 4 years ago

Previously I was under the impression that fallbacks like [] throw since there is no resolution found? And that null was just an alias of that?

Correct (and null and [] should behave in the same way as domenic commented https://github.com/WICG/import-maps/issues/184#issuecomment-542075266).

...Hmm, in the current spec, blocking a specific resolution is harder than before, due to two factors:

For example, with the following import map (assuming std:something exists):

{
  "imports": {
    "@notfound": "std:something",
    "@something": "std:something",
    "std:something": null,
    "/a/": "/A/",
    "/a/b/": null,
    "/a/b/c/": "/A/B/C/",
    "/a/d/": "data:text/html,/",
    "/a/d/e/": "/A/D/E/"
  },
  "scopes": {
    "/scope1/": {
      "@notfound": null,
      "std:something": null
    }
  }
}

The resolution result is:

specifier scope result fallback sequence
@something /scope1/ std:something FAIL for /scope1/ (no mapping) -> SUCCESS for top-level
@notfound /scope1/ std:something FAIL for /scope1/ (null mapping ignored) -> SUCCESS for top-level
std:something /scope1/ std:something FAIL for /scope1/ (null mapping ignored) -> FAIL for top-level (null mapping ignored) -> Resolved as-is
@something top-level std:something SUCCESS for top-level
@notfound top-level std:something SUCCESS for top-level
std:something top-level std:something FAIL for top-level (null mapping ignored) -> Resolved as-is
/a/b/c/foo * /A/B/C/foo SUCCESS for /a/b/c/
/a/b/foo * /A/b/foo FAIL for /a/b/ -> SUCCESS for /a/
/a/foo * /A/foo SUCCESS for /a/
/a/d/e/foo * /A/D/E/foo SUCCESS for /a/d/e/
/a/d/foo * /a/d/foo FAIL for /a/d/ -> (/a/ is skipped) -> Resolved as-is
hiroshige-g commented 4 years ago

If we want to definitely block a resolution by using null mappings, then it's better to consider null mapping separately from other failures. For example, resolution result (for each scope, each prefix, etc.) should be either:

hiroshige-g commented 4 years ago

(FYI I removed the blocking dependency from test migration to this issue. This is still blocking shipping, but not immediate CLs)

hiroshige-g commented 4 years ago

Preparing a spec PR.