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

Parsing: fail hard or fail soft? #80

Closed
domenic opened this issue Nov 27, 2018 · 7 comments · Fixed by #151
Closed

Parsing: fail hard or fail soft? #80

domenic opened this issue Nov 27, 2018 · 7 comments · Fixed by #151
Assignees
Milestone

Comments

@domenic
Copy link
Collaborator

domenic commented Nov 27, 2018

(Originally brought up by @guybedford in #67.)

In the reference implementation so far (see also tests), parsing is forgiving in several cases:

  1. Top-level entries besides "imports" or "scopes" are ignored.
  2. Addresses that are not strings, null, or an array cause the corresponding map entry to be ignored.
  3. Empty string specifier keys cause the corresponding map entry to be ignored.
  4. If an address is an array, members that are not strings are ignored.
  5. String addresses that are not absolute URLs with fetch schemes, relative-URL-like, or built-in module specifiers are ignored.
  6. Unparseable scope prefix URLs, or scope prefix URLs with non-fetch schemes, are ignored.

Note that "ignored" does not preclude "shows a warning in the developer console"; it just means "does not cause the entire map to be rejected".

Should we turn any of these cases into hard failures, that cause the entire map to be rejected?

My reasoning is that soft failures are more future-extensible. For example, if we introduce a new top-level key for some optional nice feature, it would be unfortunate if using it caused your import map to be completely rejected in browsers that haven't yet implemented that feature. Similarly if we introduce an address format that's an object, or we introduce a new URL scheme, or we start allowing non-built-in module specifiers as addresses.

Does anyone want to make an argument for hard failure, in any of those six cases? If so, let's discuss. If not, we can consider this issue closed when I've added committed documentation with reasoning and plausible example cases for each of the above soft failures. Eventually that documentation would go into the spec.

@ljharb
Copy link

ljharb commented Nov 27, 2018

Is there some kind of middle ground, where a map could opt into, or out of, strict parsing? iow, if i want the errors, or i don’t, i can still get that behavior.

@domenic
Copy link
Collaborator Author

domenic commented Nov 27, 2018

I think we'd call that a lint tool ;)

@ljharb
Copy link

ljharb commented Nov 27, 2018

Fair enough, but it still seems valuable to have that capability built in somehow.

@guybedford
Copy link
Collaborator

My primary concern here was that the debugging experience is not awful, which was an important lesson from SystemJS.

As long as the soft errors / validation failures are clearly described in the spec as something that should be provided to users (as console warnings or otherwise), I'd be ok with this.

While many browsers might not emit these initially, we get there eventually which is the important point. I agree that hard errors may inhibit future additions so that it makes more sense on balance.

@guybedford
Copy link
Collaborator

To be clear, I'm saying that all of 1 - 6 should be console warnings. Anything that results in the parser or implementation ignoring that field or entry should warn.

domenic pushed a commit that referenced this issue Jan 18, 2019
If an entry has an address with no trailing slash, but the specifier key has a trailing slash, then discard the address. Closes #93.

This also introduces warning infrastructure, as discussed in #80, and includes our first warning, for when such addresses are discarded.

A related piece of trailing-slash behavior is being discussed in #91; stay tuned for further tweaks.
domenic pushed a commit that referenced this issue Jan 18, 2019
@littledan
Copy link
Contributor

I like the "fail soft" semantics.

For one concrete use case: We may want to add more complicated tests to the fallback list in the future. One example is #61 (if it is common, it would be a shame to not expose the test that's going on to the prefetch handler), and another example is: If the initially shipped version of a module has a base set of features, and over time, some more features are added, there may be a benefit from having JS implementation of just the new feature, without a full implementation of the whole thing. Maybe something based on get-originals could be used for feature testing queries.

(This scenario is not just about new features added over time; sometimes browsers ship partial implementations. For this reason, version numbers would not work for these queries in general.)

I don't we should add a complicated feature testing system should to import-maps out of the gate; it's probably best to take it incrementally, to solve for the kinds of issues that come up. If we skip over non-strings in the fallback list, that gives us space to expand in the future, to make some of the entries be objects that contain information about the conditional test.

@domenic
Copy link
Collaborator Author

domenic commented Jul 3, 2019

The work left to do here is to add warnings to the reference implementation/spec/tests for all of the cases in the OP. (Plus any that have appeared since then?) I think we have 2-3 warnings already but not all of them.

@domenic domenic added this to the MVP milestone Jul 3, 2019
@domenic domenic self-assigned this Jul 10, 2019
domenic added a commit that referenced this issue Jul 10, 2019
domenic added a commit that referenced this issue Jul 10, 2019
domenic added a commit that referenced this issue Jul 10, 2019
domenic added a commit that referenced this issue Jul 10, 2019
domenic added a commit that referenced this issue Jul 10, 2019
Part of #80. The included specifier key parsing test ensures that no warnings are given for specifier keys that can't be parsed as URLs; instead they are left alone.
domenic added a commit that referenced this issue Jul 10, 2019
domenic added a commit that referenced this issue Jul 11, 2019
domenic added a commit that referenced this issue Jul 12, 2019
domenic added a commit that referenced this issue Jul 12, 2019
domenic added a commit that referenced this issue Jul 12, 2019
domenic added a commit that referenced this issue Jul 12, 2019
domenic added a commit that referenced this issue Jul 12, 2019
Part of #80. The included specifier key parsing test ensures that no warnings are given for specifier keys that can't be parsed as URLs; instead they are left alone.
domenic added a commit that referenced this issue Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants