-
Notifications
You must be signed in to change notification settings - Fork 72
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
Clarification: #176 and invalid URLs #184
Comments
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 Since the most natural behavior for Then, of course, So the question is what should mapping to 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. |
+1. I'm a little leaning toward ignoring mapping to
Also, even when a mapping to 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. |
This CL makes `null` entries, i.e. mappings to `null`, `[]`, invalid URLs, etc., be ignored, while previously they caused an error. Built-in-modules-enabled cases: The tests for built-in-modules-enabled cases are based on an old version of the spec where `null` entries should cause errors. This CL changes the tests so that `null` entries are ignored. Associated spec updates for this change is not needed for now, because the built-in modules support is tentatively dropped from the spec. Built-in-modules-disabled cases: The new behavior is already conformant with the current spec and WPT tests. Bug: 990561, WICG/import-maps#184 Change-Id: I3ce416adc9b9145b7fae8d9a22973698bd632f7e
Without standard modules like Previously I was under the impression that fallbacks like |
Correct (and ...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 {
"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:
|
If we want to definitely block a resolution by using
|
This CL makes `null` entries, i.e. mappings to `null`, `[]`, invalid URLs, etc., be ignored, while previously they caused an error. Built-in-modules-enabled cases: The tests for built-in-modules-enabled cases are based on an old version of the spec where `null` entries should cause errors. This CL changes the tests so that `null` entries are ignored. Associated spec updates for this change is not needed for now, because the built-in modules support is tentatively dropped from the spec. Built-in-modules-disabled cases: The new behavior is already conformant with the current spec and WPT tests. Bug: 990561, WICG/import-maps#184 Change-Id: I3ce416adc9b9145b7fae8d9a22973698bd632f7e
(FYI I removed the blocking dependency from test migration to this issue. This is still blocking shipping, but not immediate CLs) |
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/master@{#731910}
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/master@{#731910}
This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/master@{#731910}
… tests into JSON-based, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based parsing tests into JSON-based This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/master@{#731910} -- wpt-commits: a5e2efd90b6766f33e94d92e1858a68f8c40c977 wpt-pr: 21064
… tests into JSON-based, a=testonly Automatic update from web-platform-tests [Import Maps] Migrate Jest-based parsing tests into JSON-based This CL - Converts Jest-based parsing tests into JSONs (with minor refinement, and removing test cases depending on interoperability issues of underlying URL parsers), - Adjust the JSON schema for parsing tests, and - Removes imported resolution tests. The test failures (virtual/import-maps-without-builtin-modules/external/wpt/import-maps/common/parsing.tentative.html) are due to WICG/import-maps#184, i.e. whether `null` mappings are removed from parsed import map or not. Bug: 1026809, WICG/import-maps#170 Change-Id: If1e53cbeafd44c1de1a7fa38ef646fcd5721b495 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982509 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/master@{#731910} -- wpt-commits: a5e2efd90b6766f33e94d92e1858a68f8c40c977 wpt-pr: 21064
Preparing a spec PR. |
After this change, all errors, found either at parsing or resolution time, result in the resolution processing throwing errors. Previously, errors found at parsing time, namely: * Non-string values, including null, [], etc. * Invalid URLs * Entries with keys with trailing "/" and values without trailing "/" were ignored, allowing fallback to the next candidate at resolution time. Thus, they could not be used as a definitive way to block particular resolution. After this change, such entries are kept in the parsed specifier maps as entries with null value, and cause resolution to fail (without further fallback) by throwing errors. Also, before this change, relative URL resolution failures at prefix matching in "resolve a module specifier" didn't allow fallback to less-specific prefixes, but did allow fallback to less-specific scopes. This PR makes errors which would have caused such fallbacks always throw errors, so fallback is not allowed in either case. With this change, all resolution failures are surfaced via thrown errors, and resolution is definitively blocked. Fallback to less-specific scopes only occurs when there are no matching entries in a scope; errored entries no longer cause fallbacks. Fixes #184.
Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454
Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454
Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#740847}
Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#740847}
Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#740847}
…e resolution, Blink-side, a=testonly Automatic update from web-platform-tests [Import Maps] Make errors block the whole resolution, Blink-side Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#740847} -- wpt-commits: 8767abfffb790e4928ac9433282928caa2357ee8 wpt-pr: 21707
…e resolution, Blink-side, a=testonly Automatic update from web-platform-tests [Import Maps] Make errors block the whole resolution, Blink-side Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#740847} -- wpt-commits: 8767abfffb790e4928ac9433282928caa2357ee8 wpt-pr: 21707
…e resolution, Blink-side, a=testonly Automatic update from web-platform-tests [Import Maps] Make errors block the whole resolution, Blink-side Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#740847} -- wpt-commits: 8767abfffb790e4928ac9433282928caa2357ee8 wpt-pr: 21707 UltraBlame original commit: e620b95565ea737f093567c22c6b8ce07e37b7dc
…e resolution, Blink-side, a=testonly Automatic update from web-platform-tests [Import Maps] Make errors block the whole resolution, Blink-side Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#740847} -- wpt-commits: 8767abfffb790e4928ac9433282928caa2357ee8 wpt-pr: 21707 UltraBlame original commit: e620b95565ea737f093567c22c6b8ce07e37b7dc
…e resolution, Blink-side, a=testonly Automatic update from web-platform-tests [Import Maps] Make errors block the whole resolution, Blink-side Reflects WICG/import-maps#205. This CL updates tests from #205. To match the behavior with the updated spec, this CL turns non-String values into `null` entries (i.e. make whole resolution fail without further fallback), instead of ignoring such entries. Other aspects were already conformant with the updated spec (i.e. weren't matching with the spec before #205). This CL updates (test-only) import maps serialization code so that it matches with the reference implementation, i.e. dump `null` entries as `null` instead of `[]`. This CL also updates spec comments. Bug: 990561, WICG/import-maps#184 Change-Id: Ifa2d04bf20fcef5575c14d135c328730ea09c454 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2037012 Commit-Queue: Hiroshige Hayashizaki <hiroshigechromium.org> Reviewed-by: Kouhei Ueno <kouheichromium.org> Cr-Commit-Position: refs/heads/master{#740847} -- wpt-commits: 8767abfffb790e4928ac9433282928caa2357ee8 wpt-pr: 21707 UltraBlame original commit: e620b95565ea737f093567c22c6b8ce07e37b7dc
#176 changed the behavior around invalid URLs. i.e.
{"imports": { "https://example.com/foo": "https://:invalid:url" }}
is equal to
{"imports": {}}
after Slim down to a minimal core #176, i.e."https://example.com/foo"
remains unmapped (i.e. import maps can't be used to force-reject specifier resolution), while{"imports": { "https://example.com/foo": null }}
before Slim down to a minimal core #176, i.e. resolving"https://example.com/foo"
fails.What was the reason behind this change?
Does this mean that, after #176, when we re-enable fallback/built-in modules support again:
{"imports": { "https://example.com/foo": "https://:invalid:url" }}
is equal to{"imports": {}}
as a direct result of the change Slim down to a minimal core #176,{"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 Slim down to a minimal core #176,{"imports": { "https://example.com/foo": [] }}
is equal to{"imports": {}}
,{"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.
The text was updated successfully, but these errors were encountered: