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

[hiroshige-g]

This PR all errors (found either at parsing or resolution time) throw errors at resolution.

Before this PR, 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 next candidates at resolution time, and thus can't be used as a definitive way to block particular resolution.
After this PR, such entries are kept in specifier maps as entries with null value, and cause resolutions fail (without further fallback) by throwing errors.
For thus purpose, this PR changes the type of values of specifier maps to URL or null.

Also, before this PR, relative URL resolution failures at prefix matching in #resolve-a-module-specifier didn't allow fallback to less specific prefixes while allowing fallback to less specific scopes.
This PR makes such errors throw errors, disallowing fallback to less specific prefixes and scopes.

This PR unifies resolution failures into one: throwing an error.
After this PR, all kind of errors throw at resolution time and definitively blocking resolution.
Fallback to less specific scopes occur only when there are no matching entries in a scope.

Fixes #184.

---

Preview | Diff

[hiroshige-g]

I'll create an associated test update PR later (due to dependency to #204; drafted locally).

[domenic]

This looks great to me. I especially appreciate the detailed reasoning in the commit message.

I have some nits I would like to tweak in the spec wording/formatting, so when you think things are good to go I will push those and merge.

[hiroshige-g]

Added/updated ref impl and tests. Ready for review.

[domenic]

I pushed some tweaks to the spec text; please take a look. The only significant one is adding a third bullet to your note explaining the "resolving module specifiers" section.

However, reviewing this I noticed one thing that we should correct before merging. The reference implementation and the spec use rather different algorithms; there are more early returns/throws in the reference implementation, and the variable names were not updated.

I imagine the algorithms are equivalent. But we should still align these, as part of the whole idea of the reference implementation is to validate that the spec algorithms give the expected results. I'm just not sure which is better, i.e. should we align the spec with the reference implementation or the reference implementation with the spec. Do you have a preference?

[hiroshige-g]

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?

[domenic]

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.

?

[hiroshige-g]

"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"

?

[hiroshige-g]

The reference implementation

should we align the spec with the reference implementation or the reference implementation with the spec.

One point I'm aware (and I was not sure) is Step 1.4-1.5 of #resolve-an-imports-match.
In the spec, all control paths sets resolutionResult and then Step 1.4-1.5 handle resolutionResult in a centralized fashion,
while in the ref impl each cases immediately throws etc.

Pros of ref impl style:

• Each case can provide more detailed error messages attached with errors, depending on contexts.

Cons of ref impl style:

• It's not clear that there are only three kind of resolution results for each entry, because there are no centralized result handling (like Step 1.4-1.5 in the spec).

Probably I should further refactor both the spec and implementation. I will try tomorow.

Or are there other diffs?

[domenic]

It's not clear that there are only three kind of resolution results for each entry

I don't think this is super clear even from the spec style. (The note attempts to clarify it, but as seen from our above discussion, the exact correspondence between those three cases and different steps in the algorithms is not 1:1.) So, I think I lean toward the reference implementation style.

[hiroshige-g]

+1 to the reference implementation style.
I tried to refactor the spec to clarify the invariant, but it didn't turned successful (needed more plumbings, subalgorithms etc.).

Preparing a commit...

[hiroshige-g]

Pushed. WDYT?

[domenic]

I pushed one more commit to make the reference implementation and spec 100% align. Let me know if it looks good and if so we can merge.

[hiroshige-g]

Looks good!