Skip to content
This repository was archived by the owner on Feb 26, 2025. It is now read-only.

Commit

Permalink
Allow slashes in built-in module specifiers
Browse files Browse the repository at this point in the history 
Closes #129.
  • Loading branch information
@domenic
domenic authored Jul 30, 2019
1 parent da5665c commit d5add26
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 53 deletions.
53 changes: 32 additions & 21 deletions reference-implementation/__tests__/parsing-addresses.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,23 +116,19 @@ describe('Built-in module addresses', () => {
);
});

it('should ignore and warn on built-in module URLs that contain "/"', () => {
it('should allow built-in module URLs that contain "/" or "\\"', () => {
expectSpecifierMap(
`{
"bad1": "${BUILT_IN_MODULE_SCHEME}:foo/",
"bad2": "${BUILT_IN_MODULE_SCHEME}:foo/bar",
"good": "${BUILT_IN_MODULE_SCHEME}:foo\\\\baz"
"slashEnd": "${BUILT_IN_MODULE_SCHEME}:foo/",
"slashMiddle": "${BUILT_IN_MODULE_SCHEME}:foo/bar",
"backslash": "${BUILT_IN_MODULE_SCHEME}:foo\\\\baz"
}`,
'https://base.example/path1/path2/path3',
{
bad1: [],
bad2: [],
good: [expect.toMatchURL(`${BUILT_IN_MODULE_SCHEME}:foo\\baz`)]
},
[
`Invalid address "${BUILT_IN_MODULE_SCHEME}:foo/". Built-in module URLs must not contain "/".`,
`Invalid address "${BUILT_IN_MODULE_SCHEME}:foo/bar". Built-in module URLs must not contain "/".`
]
slashEnd: [expect.toMatchURL(`${BUILT_IN_MODULE_SCHEME}:foo/`)],
slashMiddle: [expect.toMatchURL(`${BUILT_IN_MODULE_SCHEME}:foo/bar`)],
backslash: [expect.toMatchURL(`${BUILT_IN_MODULE_SCHEME}:foo\\baz`)]
}
);
});
});
Expand Down Expand Up @@ -285,39 +281,54 @@ describe('Failing addresses: mismatched trailing slashes', () => {
it('should warn for the simple case', () => {
expectSpecifierMap(
`{
"trailer/": "/notrailer"
"trailer/": "/notrailer",
"${BUILT_IN_MODULE_SCHEME}:trailer/": "/bim-notrailer"
}`,
'https://base.example/path1/path2/path3',
{
'trailer/': []
'trailer/': [],
[`${BUILT_IN_MODULE_SCHEME}:trailer/`]: []
},
[`Invalid address "https://base.example/notrailer" for package specifier key "trailer/". Package addresses must end with "/".`]
[
`Invalid address "https://base.example/notrailer" for package specifier key "trailer/". Package addresses must end with "/".`,
`Invalid address "https://base.example/bim-notrailer" for package specifier key "${BUILT_IN_MODULE_SCHEME}:trailer/". Package addresses must end with "/".`
]
);
});

it('should warn for a mismatch alone in an array', () => {
expectSpecifierMap(
`{
"trailer/": ["/notrailer"]
"trailer/": ["/notrailer"],
"${BUILT_IN_MODULE_SCHEME}:trailer/": ["/bim-notrailer"]
}`,
'https://base.example/path1/path2/path3',
{
'trailer/': []
'trailer/': [],
[`${BUILT_IN_MODULE_SCHEME}:trailer/`]: []
},
[`Invalid address "https://base.example/notrailer" for package specifier key "trailer/". Package addresses must end with "/".`]
[
`Invalid address "https://base.example/notrailer" for package specifier key "trailer/". Package addresses must end with "/".`,
`Invalid address "https://base.example/bim-notrailer" for package specifier key "${BUILT_IN_MODULE_SCHEME}:trailer/". Package addresses must end with "/".`
]
);
});

it('should warn for a mismatch alongside non-mismatches in an array', () => {
expectSpecifierMap(
`{
"trailer/": ["/atrailer/", "/notrailer"]
"trailer/": ["/atrailer/", "/notrailer"],
"${BUILT_IN_MODULE_SCHEME}:trailer/": ["/bim-atrailer/", "/bim-notrailer"]
}`,
'https://base.example/path1/path2/path3',
{
'trailer/': [expect.toMatchURL('https://base.example/atrailer/')]
'trailer/': [expect.toMatchURL('https://base.example/atrailer/')],
[`${BUILT_IN_MODULE_SCHEME}:trailer/`]: [expect.toMatchURL('https://base.example/bim-atrailer/')]
},
[`Invalid address "https://base.example/notrailer" for package specifier key "trailer/". Package addresses must end with "/".`]
[
`Invalid address "https://base.example/notrailer" for package specifier key "trailer/". Package addresses must end with "/".`,
`Invalid address "https://base.example/bim-notrailer" for package specifier key "${BUILT_IN_MODULE_SCHEME}:trailer/". Package addresses must end with "/".`
]
);
});
});
Expand Down
10 changes: 4 additions & 6 deletions reference-implementation/__tests__/parsing-specifier-keys.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ describe('Absolute URL specifier keys', () => {
);
});

it('should only parse built-in module specifier keys without a /', () => {
it('should parse built-in module specifier keys, including with a "/"', () => {
expectSpecifierMap(
`{
"${BLANK}": "/blank",
Expand All @@ -150,12 +150,10 @@ describe('Absolute URL specifier keys', () => {
'https://base.example/path1/path2/path3',
{
[BLANK]: [expect.toMatchURL('https://base.example/blank')],
[`${BLANK}/`]: [expect.toMatchURL('https://base.example/blank/')],
[`${BLANK}/foo`]: [expect.toMatchURL('https://base.example/blank/foo')],
[`${BLANK}\\foo`]: [expect.toMatchURL('https://base.example/blank/backslashfoo')]
},
[
`Invalid specifier key "${BLANK}/". Built-in module specifiers must not contain "/".`,
`Invalid specifier key "${BLANK}/foo". Built-in module specifiers must not contain "/".`
]
}
);
});
});
54 changes: 54 additions & 0 deletions reference-implementation/__tests__/resolving-builtins.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,24 @@ describe('Remapping built-in module specifiers', () => {
expect(resolveUnderTest(NONE)).toMatchURL('https://example.com/app/none.mjs');
});

it('should remap built-in modules with slashes', () => {
const resolveUnderTest = makeResolveUnderTest(`{
"imports": {
"${BLANK}/": "./blank-slash/",
"${BLANK}/foo": "./blank-foo.mjs",
"${NONE}/": "./none-slash/",
"${NONE}/foo": "./none-foo.mjs"
}
}`);

expect(resolveUnderTest(`${BLANK}/`)).toMatchURL('https://example.com/app/blank-slash/');
expect(resolveUnderTest(`${BLANK}/foo`)).toMatchURL('https://example.com/app/blank-foo.mjs');
expect(resolveUnderTest(`${BLANK}/bar`)).toMatchURL('https://example.com/app/blank-slash/bar');
expect(resolveUnderTest(`${NONE}/`)).toMatchURL('https://example.com/app/none-slash/');
expect(resolveUnderTest(`${NONE}/foo`)).toMatchURL('https://example.com/app/none-foo.mjs');
expect(resolveUnderTest(`${NONE}/bar`)).toMatchURL('https://example.com/app/none-slash/bar');
});

it('should remap built-in modules with fallbacks', () => {
const resolveUnderTest = makeResolveUnderTest(`{
"imports": {
Expand All @@ -51,13 +69,40 @@ describe('Remapping built-in module specifiers', () => {
expect(resolveUnderTest(BLANK)).toMatchURL(BLANK);
expect(resolveUnderTest(NONE)).toMatchURL('https://example.com/app/none.mjs');
});

it('should remap built-in modules with slashes and fallbacks', () => {
// NOTE: `${BLANK}/for-testing` is not per spec, just for these tests.
// See resolver.js.
const resolveUnderTest = makeResolveUnderTest(`{
"imports": {
"${BLANK}/": ["${BLANK}/", "./blank/"],
"${BLANK}/for-testing": ["${BLANK}/for-testing", "./blank-for-testing-special"],
"${NONE}/": ["${NONE}/", "./none/"],
"${NONE}/foo": ["${NONE}/foo", "./none-foo-special"]
}
}`);

// Built-in modules only resolve for exact matches, so this will trigger the fallback.
expect(resolveUnderTest(`${BLANK}/`)).toMatchURL('https://example.com/app/blank/');
expect(resolveUnderTest(`${BLANK}/foo`)).toMatchURL('https://example.com/app/blank/foo');

// This would fall back in a real implementation; it's only because we've gone against
// spec in the reference implementation (to make this testable) that this maps.
expect(resolveUnderTest(`${BLANK}/for-testing`)).toMatchURL(`${BLANK}/for-testing`);

expect(resolveUnderTest(`${NONE}/`)).toMatchURL('https://example.com/app/none/');
expect(resolveUnderTest(`${NONE}/bar`)).toMatchURL('https://example.com/app/none/bar');
expect(resolveUnderTest(`${NONE}/foo`)).toMatchURL('https://example.com/app/none-foo-special');
});
});

describe('Remapping to built-in modules', () => {
const resolveUnderTest = makeResolveUnderTest(`{
"imports": {
"blank": "${BLANK}",
"/blank": "${BLANK}",
"/blank/": "${BLANK}/",
"/blank-for-testing": "${BLANK}/for-testing",
"none": "${NONE}",
"/none": "${NONE}"
}
Expand All @@ -68,6 +113,15 @@ describe('Remapping to built-in modules', () => {
expect(resolveUnderTest('/blank')).toMatchURL(BLANK);
});

it(`should fail when remapping to "${BLANK}/"`, () => {
expect(() => resolveUnderTest('/blank/')).toThrow(TypeError);
});

it(`should remap to "${BLANK}/for-testing"`, () => {
expect(resolveUnderTest('/blank/for-testing')).toMatchURL(`${BLANK}/for-testing`);
expect(resolveUnderTest('/blank-for-testing')).toMatchURL(`${BLANK}/for-testing`);
});

it(`should remap to "${BLANK}" for URL-like specifiers`, () => {
expect(resolveUnderTest('/blank')).toMatchURL(BLANK);
expect(resolveUnderTest('https://example.com/blank')).toMatchURL(BLANK);
Expand Down
14 changes: 2 additions & 12 deletions reference-implementation/lib/parser.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';
const assert = require('assert');
const { tryURLParse, hasFetchScheme, tryURLLikeSpecifierParse, BUILT_IN_MODULE_PROTOCOL } = require('./utils.js');
const { tryURLParse, hasFetchScheme, tryURLLikeSpecifierParse } = require('./utils.js');

exports.parseFromString = (input, baseURL) => {
const parsed = JSON.parse(input);
Expand Down Expand Up @@ -86,11 +86,6 @@ function sortAndNormalizeSpecifierMap(obj, baseURL) {
continue;
}

if (addressURL.protocol === BUILT_IN_MODULE_PROTOCOL && addressURL.href.includes('/')) {
console.warn(`Invalid address "${potentialAddress}". Built-in module URLs must not contain "/".`);
continue;
}

validNormalizedAddresses.push(addressURL);
}
normalized[specifierKey] = validNormalizedAddresses;
Expand Down Expand Up @@ -145,12 +140,7 @@ function normalizeSpecifierKey(specifierKey, baseURL) {

const url = tryURLLikeSpecifierParse(specifierKey, baseURL);
if (url !== null) {
const urlString = url.href;
if (url.protocol === BUILT_IN_MODULE_PROTOCOL && urlString.includes('/')) {
console.warn(`Invalid specifier key "${urlString}". Built-in module specifiers must not contain "/".`);
return null;
}
return urlString;
return url.href;
}

return specifierKey;
Expand Down
29 changes: 27 additions & 2 deletions reference-implementation/lib/resolver.js
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
'use strict';
const { URL } = require('url');
const assert = require('assert');
const { tryURLLikeSpecifierParse, BUILT_IN_MODULE_SCHEME, BUILT_IN_MODULE_PROTOCOL } = require('./utils.js');

const supportedBuiltInModules = new Set([`${BUILT_IN_MODULE_SCHEME}:blank`]);
// TODO: clean up by allowing caller (and thus tests) to choose the list of built-ins?
const supportedBuiltInModules = new Set([
`${BUILT_IN_MODULE_SCHEME}:blank`,
`${BUILT_IN_MODULE_SCHEME}:blank/for-testing` // NOTE: not in the spec.
]);

exports.resolve = (specifier, parsedImportMap, scriptURL) => {
const asURL = tryURLLikeSpecifierParse(specifier, scriptURL);
Expand Down Expand Up @@ -64,7 +69,27 @@ function resolveImportsMatch(normalizedSpecifier, specifierMap) {
`(via prefix specifier key "${specifierKey}").`);
} else if (addresses.length === 1) {
const afterPrefix = normalizedSpecifier.substring(specifierKey.length);
return new URL(afterPrefix, addresses[0]);

// Enforced by parsing
assert(addresses[0].href.endsWith('/'));

// Cannot use URL resolution directly. E.g. "switch" relative to "std:elements/" is a
// parsing failure.
return new URL(addresses[0] + afterPrefix);
} else if (addresses.length === 2 &&
addresses[0].protocol === BUILT_IN_MODULE_PROTOCOL &&
addresses[1].protocol !== BUILT_IN_MODULE_PROTOCOL) {
const afterPrefix = normalizedSpecifier.substring(specifierKey.length);

// Enforced by parsing
assert(addresses[0].href.endsWith('/'));
assert(addresses[1].href.endsWith('/'));

// Cannot use URL resolution directly. E.g. "switch" relative to "std:elements/" is a
// parsing failure.
const url0 = new URL(addresses[0] + afterPrefix);
const url1 = new URL(addresses[1] + afterPrefix);
return supportedBuiltInModules.has(url0.href) ? url0 : url1;
} else {
throw new Error('The reference implementation for multi-address fallbacks that are not ' +
'[built-in module, fetch-scheme URL] is not yet implemented.');
Expand Down
24 changes: 12 additions & 12 deletions spec.bs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,9 +398,6 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:
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. [=Continue=].
1. If |specifierKey|'s [=url/scheme=] is "`std`" and the [=URL serializer|serialization=] of |addressURL| contains U+002F (/), then:
1. [=Report a warning to the console=] that built-in module URLs must not contain slashes.
1. [=Continue=].
1. [=list/Append=] |addressURL| to |validNormalizedAddresses|.
1. Set |normalized|[|specifierKey|] to |validNormalizedAddresses|.
1. Return the result of [=map/sorting=] |normalized|, with an entry |a| being less than an entry |b| if |a|'s [=map/key=] is [=longer or code unit less than=] |b|'s [=map/key=].
Expand Down Expand Up @@ -431,12 +428,7 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:
1. [=Report a warning to the console=] that specifier keys cannot be the empty string.
1. Return null.
1. Let |url| be the result of [=parsing a URL-like import specifier=], given |specifierKey| and |baseURL|.
1. If |url| is not null, then:
1. Let |urlString| be the [=URL serializer|serialization=] of |url|.
1. If |url|'s [=url/scheme=] is "`std`" and |urlString| contains U+002F (/), then:
1. [=Report a warning to the console=] that built-in module specifiers must not contain slashes.
1. Return null.
1. Return |urlString|.
1. If |url| is not null, then return the [=URL serializer|serialization=] of |url|.
1. Return |specifierKey|.
</div>

Expand Down Expand Up @@ -511,9 +503,18 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:
1. If |addresses|'s [=list/size=] is 0, then throw a {{TypeError}} indicating that |normalizedSpecifier| was mapped to no addresses.
1. If |addresses|'s [=list/size=] is 1, then:
1. Let |afterPrefix| be the portion of |normalizedSpecifier| after the initial |specifierKey| prefix.
1. Let |url| be the result of [=URL parser|parsing=] |afterPrefix| relative to |addresses|[0].
1. If |url| is failure, throw a {{TypeError}}, implicating |normalizedSpecifier| (and in particular the |afterPrefix| portion).
1. Assert: |afterPrefix| ends with "`/`", as enforced during [=parse an import map string|parsing=].
1. Let |url| be the result of [=URL parser|parsing=] the concatenation of the [=URL serializer|serialization=] of |addresses|[0] with |afterPrefix|.
<p class="note">We [=URL parser|parse=] the concatenation, instead of parsing |afterPrefix| relative to |addresses|[0], due to cases such as an |afterPrefix| of "`switch`" and an |addresses|[0] of "`std:elements/`".
1. Assert: |url| is not failure, since |addresses|[0] was a URL, and appending after the trailing "`/`" will not make it unparseable.
1. Return |url|.
1. If |addresses|'s [=list/size=] is 2, and |addresses|[0]'s [=url/scheme=] is "`std`", and |addresses|[1]'s [=url/scheme=] is <em>not</em> "`std`", 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 |url0| be the result of [=URL parser|parsing=] the concatenation of the [=URL serializer|serialization=] of |addresses|[0] with |afterPrefix|; similarly, let |url1| be the result of [=URL parser|parsing=] the concatenation of the [=URL serializer|serialization=] of |addresses|[1] with |afterPrefix|.
<p class="note">As above, we parse the concatenation to deal with built-in module cases.
1. Assert: neither |url0| nor |url1| are failure, since |addresses|[0] and |addresses|[1] were URLs, and appending after their trailing "`/`" will not make them unparseable.
1. Return |url0|, if |moduleMap|[|url0|] [=map/exists=]; otherwise, return |url1|.
1. Otherwise, <span class="advisement">we have no specification for more complicated fallbacks yet; throw a {{TypeError}} indicating this is not yet supported</span>.
1. Return null.
</div>
Expand All @@ -522,7 +523,6 @@ To <dfn>register an import map</dfn> given an {{HTMLScriptElement}} |element|:
To <dfn lt="validate the module script URL|validate a module script URL">validate a module script URL</dfn>, given a [=URL=] |url|, an [=environment settings object=] |settings object|, and a [=URL=] |base URL|:

1. If |url|'s [=url/scheme=] is "`std`", then:
1. If the [=URL serializer|serialization=] of |url| contains U+002F (/), then throw a {{TypeError}} indicating that |url| is a malformed built-in URL.
1. Let |moduleMap| be |settings object|'s [=environment settings object/module map=].
1. If |moduleMap|[|url|] does not [=map/exist=], then throw a {{TypeError}} indicating that the requested built-in module is not implemented.
<p class="note">This condition is added to ensure that |moduleMap|[|url|] does not [=map/exist=] for unimplemented built-ins. Without this condition, <a spec="html">fetch a single module script</a> might be called and |moduleMap|[|url|] can be set to null, which might complicates the spec around built-ins.</p>
Expand Down

0 comments on commit d5add26

Please sign in to comment.