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

Make any errors (invalid URLs etc) throw errors in resolution #205

Merged
merged 9 commits into from
Feb 7, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
"importMapBaseURL": "https://base.example/path1/path2/path3",
"expectedParsedImportMap": {
"imports": {
"unparseable2": null,
"unparseable3": null,
"invalidButParseable1": "https://example.org/",
"invalidButParseable2": "https://example.com///",
"prettyNormal": "https://example.net/",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,13 @@
},
"importMapBaseURL": "https://base.example/path1/path2/path3",
"expectedParsedImportMap": {
"imports": {},
"imports": {
"foo1": null,
"foo2": null,
"foo3": null,
"foo4": null,
"foo5": null
},
"scopes": {}
}
}
Expand Down
16 changes: 14 additions & 2 deletions reference-implementation/__tests__/json/parsing-addresses.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@
},
"importMapBaseURL": "data:text/html,test",
"expectedParsedImportMap": {
"imports": {},
"imports": {
"dotSlash": null,
"dotDotSlash": null,
"slash": null
},
"scopes": {}
}
},
Expand Down Expand Up @@ -65,7 +69,15 @@
},
"importMapBaseURL": "https://base.example/path1/path2/path3",
"expectedParsedImportMap": {
"imports": {},
"imports": {
"dotSlash1": null,
"dotDotSlash1": null,
"dotSlash2": null,
"dotDotSlash2": null,
"slash2": null,
"dotSlash3": null,
"dotDotSlash3": null
},
"scopes": {}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
},
"expectedParsedImportMap": {
"imports": {
"null": null,
"boolean": null,
"number": null,
"object": null,
"array": null,
"array2": null,
"string": "https://example.com/"
},
"scopes": {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
},
"importMapBaseURL": "https://base.example/path1/path2/path3",
"expectedParsedImportMap": {
"imports": {},
"imports": {
"trailer/": null
},
"scopes": {}
}
}
82 changes: 82 additions & 0 deletions reference-implementation/__tests__/json/resolving-null.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
{
"importMapBaseURL": "https://example.com/app/index.html",
"baseURL": "https://example.com/js/app.mjs",
"name": "Entries with errors shouldn't allow fallback",
"tests": {
"No fallback to less-specific prefixes": {
"importMap": {
"imports": {
"null/": "/1/",
"null/b/": null,
"null/b/c/": "/1/2/",
"invalid-url/": "/1/",
"invalid-url/b/": "https://:invalid-url:/",
"invalid-url/b/c/": "/1/2/",
"without-trailing-slashes/": "/1/",
"without-trailing-slashes/b/": "/x",
"without-trailing-slashes/b/c/": "/1/2/",
"prefix-resolution-error/": "/1/",
"prefix-resolution-error/b/": "data:text/javascript,/",
"prefix-resolution-error/b/c/": "/1/2/"
}
},
"expectedResults": {
"null/x": "https://example.com/1/x",
"null/b/x": null,
"null/b/c/x": "https://example.com/1/2/x",
"invalid-url/x": "https://example.com/1/x",
"invalid-url/b/x": null,
"invalid-url/b/c/x": "https://example.com/1/2/x",
"without-trailing-slashes/x": "https://example.com/1/x",
"without-trailing-slashes/b/x": null,
"without-trailing-slashes/b/c/x": "https://example.com/1/2/x",
"prefix-resolution-error/x": "https://example.com/1/x",
"prefix-resolution-error/b/x": null,
"prefix-resolution-error/b/c/x": "https://example.com/1/2/x"
}
},
"No fallback to less-specific scopes": {
"importMap": {
"imports": {
"null": "https://example.com/a",
"invalid-url": "https://example.com/b",
"without-trailing-slashes/": "https://example.com/c/",
"prefix-resolution-error/": "https://example.com/d/"
},
"scopes": {
"/js/": {
"null": null,
"invalid-url": "https://:invalid-url:/",
"without-trailing-slashes/": "/x",
"prefix-resolution-error/": "data:text/javascript,/"
}
}
},
"expectedResults": {
"null": null,
"invalid-url": null,
"without-trailing-slashes/x": null,
"prefix-resolution-error/x": null
}
},
"No fallback to absolute URL parsing": {
"importMap": {
"imports": {},
"scopes": {
"/js/": {
"https://example.com/null": null,
"https://example.com/invalid-url": "https://:invalid-url:/",
"https://example.com/without-trailing-slashes/": "/x",
"https://example.com/prefix-resolution-error/": "data:text/javascript,/"
}
}
},
"expectedResults": {
"https://example.com/null": null,
"https://example.com/invalid-url": null,
"https://example.com/without-trailing-slashes/x": null,
"https://example.com/prefix-resolution-error/x": null
}
}
}
}
1 change: 1 addition & 0 deletions reference-implementation/__tests__/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ for (const jsonFile of [
'parsing-scope-keys.json',
'parsing-specifier-keys.json',
'parsing-trailing-slashes.json',
'resolving-null.json',
'scopes-exact-vs-prefix.json',
'scopes.json',
'tricky-specifiers.json',
Expand Down
3 changes: 3 additions & 0 deletions reference-implementation/lib/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,21 @@ function sortAndNormalizeSpecifierMap(obj, baseURL) {
if (typeof value !== 'string') {
console.warn(`Invalid address ${JSON.stringify(value)} for the specifier key "${specifierKey}". ` +
`Addresses must be strings.`);
normalized[normalizedSpecifierKey] = null;
continue;
}

const addressURL = tryURLLikeSpecifierParse(value, baseURL);
if (addressURL === null) {
console.warn(`Invalid address "${value}" for the specifier key "${specifierKey}".`);
normalized[normalizedSpecifierKey] = null;
continue;
}

if (specifierKey.endsWith('/') && !addressURL.href.endsWith('/')) {
console.warn(`Invalid address "${addressURL.href}" for package specifier key "${specifierKey}". ` +
`Package addresses must end with "/".`);
normalized[normalizedSpecifierKey] = null;
continue;
}

Expand Down
18 changes: 12 additions & 6 deletions reference-implementation/lib/resolver.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,19 @@ exports.resolve = (specifier, parsedImportMap, scriptURL) => {

function resolveImportsMatch(normalizedSpecifier, specifierMap) {
for (const [specifierKey, address] of Object.entries(specifierMap)) {
// Exact-match case
if (specifierKey === normalizedSpecifier) {
return address;
}
// Exact-match case
if (address === null) {
throw new TypeError(`Blocked by a null entry for "${specifierKey}"`);
} else {
return address;
}
} else if (specifierKey.endsWith('/') && normalizedSpecifier.startsWith(specifierKey)) {
// Package prefix-match case
if (address === null) {
throw new TypeError(`Blocked by a null entry for "${specifierKey}"`);
}

// Package prefix-match case
if (specifierKey.endsWith('/') && normalizedSpecifier.startsWith(specifierKey)) {
const afterPrefix = normalizedSpecifier.substring(specifierKey.length);

// Enforced by parsing
Expand All @@ -48,7 +54,7 @@ function resolveImportsMatch(normalizedSpecifier, specifierMap) {

// This code looks stupid but it follows the spec more exactly and also gives code coverage a chance to shine.
hiroshige-g marked this conversation as resolved.
Show resolved Hide resolved
if (url === null) {
return null;
throw new TypeError(`Failed to resolve prefix-match relative URL for "${specifierKey}"`);
}
return url;
}
Expand Down
37 changes: 28 additions & 9 deletions spec.bs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ summary {

<h2 id="definitions">Definitions</h2>

A <dfn>specifier map</dfn> is an [=ordered map=] from [=strings=] to [=URLs=].
A <dfn>resolution result</dfn> is either a [=URL=] or null.

A <dfn>specifier map</dfn> is an [=ordered map=] from [=strings=] to [=resolution results=].

A <dfn>import map</dfn> is a [=struct=] with two [=struct/items=]:

Expand Down Expand Up @@ -310,13 +312,16 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:
1. If |normalizedSpecifierKey| is null, then [=continue=].
1. If |value| is not a [=string=], then:
1. [=Report a warning to the console=] that addresses must be strings.
1. Set |normalized|[|specifierKey|] to null.
1. [=Continue=].
1. Let |addressURL| be the result of [=parsing a URL-like import specifier=] given |value| and |baseURL|.
1. If |addressURL| is null, then:
1. [=Report a warning to the console=] that the address was invalid.
1. Set |normalized|[|specifierKey|] to null.
1. [=Continue=].
1. If |specifierKey| ends with U+002F (/), and the [=URL serializer|serialization=] of |addressURL| does not end with U+002F (/), then:
1. [=Report a warning to the console=] that an invalid address was given for the specifier key |specifierKey|; since |specifierKey| ended in a slash, so must the address.
1. Set |normalized|[|specifierKey|] to null.
1. [=Continue=].
1. Set |normalized|[|specifierKey|] to |addressURL|.
1. Return the result of [=map/sorting=] |normalized|, with an entry |a| being less than an entry |b| if |b|'s [=map/key=] is [=code unit less than=] |a|'s [=map/key=].
Expand Down Expand Up @@ -365,6 +370,14 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:

<h2 id="resolving">Resolving module specifiers</h2>

<div class="note">
During [=resolve a module specifier|resolving a module specifier=], the following algorithms check candidate entries of [=specifier maps=], from most-specific to least-specific scopes (falling back to top-level "`imports`"), and from most-specific to least-specific prefixes. For each candidate, the result is one of the following:

- Successfully resolves a specifier to a [=URL=]. This makes the [=resolve a module specifier=] algorithm immediately return that [=URL=].
- Throws an error. This makes the [=resolve a module specifier=] algorithm rethrow the error, without any further fallbacks.
- Fail to resolve, without an error. In this case the algorithm moves on to the next candidate.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nit: I'm wondering better labels...
IIUC the third case corresponds to Step 1.3 of #resolve-an-imports-match, right?
"Throws an error" and "Fail to resolve" sound similarly and thus the difference between them might be unclear.
Alternatives I came up with are:

  • "Throws an error" and "Continue"
  • "Fail to resolve, throwing an error" and "Fail to resolve, without throwing an error"
    (Step 1.3 is just handling non-matching specifier keys, which doesn't look like failing something though)

Any idea?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking it it corresponds to both step 1.3 and step 2.

Hmm maybe

  • Resolves the specifier to null. This terminates the resolve a module specifier algorithm with a thrown exception.
  • Does not resolve the specifier. In this case the algorithm moves on to the next candidate.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"Resolves the specifier to URL", "Resolves the specifier to null" and "Does not resolve the specifier" looked great to me in terms of symmetry, but on second thought "Resolves to null" might be confusing with "Return null" in resolve an imports match (which is actually means "Does not resolve"), and might be a little inconsistent with resolve a module specifier interface (which throws exceptions rather than returning null).

"Does not resolve the specifier" still looks good to me, so how about something like:

  • "Resolves the specifier to URL", "Throws an error" and "Does not resolve the specifier"
  • "Resolves the specifier to URL", "Resolves the specifier to an error" and "Does not resolve the specifier"

?

</div>

<h3 id="new-resolve-algorithm">New "resolve a module specifier"</h3>

<div algorithm>
Expand Down Expand Up @@ -393,15 +406,21 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:
<div algorithm>
To <dfn lt="resolve an imports match|resolving an imports match">resolve an imports match</dfn>, given a [=string=] |normalizedSpecifier| and a [=specifier map=] |specifierMap|:

1. For each |specifierKey| → |address| of |specifierMap|,
1. If |specifierKey| is |normalizedSpecifier|, then return |address|.
1. If |specifierKey| ends with U+002F (/) and |normalizedSpecifier| [=/starts with=] |specifierKey|, then:
1. Let |afterPrefix| be the portion of |normalizedSpecifier| after the initial |specifierKey| prefix.
1. Assert: |afterPrefix| ends with "`/`", as enforced during [=parse an import map string|parsing=].
1. Let |url| be the result of [=URL parser|parsing=] |afterPrefix| relative to the base URL |address|.
1. If |url| is failure, then return null.
1. Return |url|.
1. For each |specifierKey| → |value| of |specifierMap|,
1. If |specifierKey| is |normalizedSpecifier|, then set |resolutionResult| to |value|.
1. Otherwise, if |specifierKey| ends with U+002F (/) and |normalizedSpecifier| [=/starts with=] |specifierKey|, then:
1. If |value| is a [=URL=]:
1. Let |afterPrefix| be the portion of |normalizedSpecifier| after the initial |specifierKey| prefix.
1. Assert: |afterPrefix| ends with "`/`", as enforced during [=parse an import map string|parsing=].
1. Let |url| be the result of [=URL parser|parsing=] |afterPrefix| relative to the base URL |value|.
1. If |url| is failure, then set |resolutionResult| to null. Otherwise, set |resolutionResult| to |url|.
1. Otherwise (i.e., if |value| is null), set |resolutionResult| to null.
1. Otherwise, [=continue=].
1. If |resolutionResult| is a [=URL=], then return |resolutionResult|.
1. Otherwise (i.e. |resolutionResult| is null), throw a {{TypeError}} indicating that resolution of |specifierKey| was blocked by an entry in an import map.
<p class="note">This will terminate the entire [=resolve a module specifier=] algorithm, without any further fallbacks.</p>
1. Return null.
<p class="note">The [=resolve a module specifier=] algorithm will fallback to a less specific scope or to "`imports`", if possible.</p>
</div>

<h3 id="resolving-updates">Updates to other algorithms</h3>
Expand Down