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

Fix minor error handling issues around specifier resolution #140

Merged
merged 3 commits into from Jun 25, 2019

Conversation

hiroshige-g
Copy link
Collaborator

@hiroshige-g hiroshige-g commented Jun 21, 2019

  • Stop returning failure (instead of null) from
    parse a URL-like import specifier.
  • Stop throwing inside resolve a module specifier algorithm, and
    return failure instead.
    Throwing an error causes e.g. throwing an error in WorkerGlobalScope
    that is not yet ready to execute scripts (
    https://html.spec.whatwg.org/C/#concept-environment-execution-ready-flag
    is false) when it occurred during fetch a module worker script graph.

Preview | Diff

- Stop returning failure (instead of null) from
  `parse a URL-like import specifier`.
- Stop throwing inside `resolve a module specifier algorithm`, and
  return failure instead.
  Throwing an error causes e.g. throwing an error in WorkerGlobalScope
  that is not yet ready to execute scripts (
  https://html.spec.whatwg.org/C/#concept-environment-execution-ready-flag
  is false) when it occurred during `fetch a module worker script graph`.
@hiroshige-g
Copy link
Collaborator Author

@domenic PTAL when you have time.

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Thanks, good catches!

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Actually, I don't think we should change the error-throwing. The spec notes

They will also need to be updated to account for it now throwing exceptions, instead of returning failure. (Previously they just turned failures into TypeErrors manually, so this is straightforward.)

You are right there are some locations we may now need to do the opposite transformation. But I think that's OK.

Also note that this PR only changes a single exception instance, whereas there are a lot of other exceptions that could be thrown from that algorithm (including by things it calls).

@hiroshige-g
Copy link
Collaborator Author

Error-throwing

Er I see, so e.g.
https://html.spec.whatwg.org/multipage/webappapis.html#creating-a-javascript-module-script
should be modified so that it catches any errors during resolve-a-module-specifier and then set script's parse error to the caught error, right?
(For most of other callers we just have to turn "assert the result is not a failure" to "assert that no errors were thrown")
I think this is fine.

@domenic
Copy link
Collaborator

domenic commented Jun 25, 2019

Right. In particular, this makes it a bit clearer for implementers reading the spec what type of error information to propagate to dynamic import()s. Instead of the algorithm receiving "failure" and turning it into a generic TypeError, it receives a TypeError that (in the spec, at least) was generated where the failure occurred.

This is not an observable difference since error messages are not specified, but I think the flow is clearer.

@domenic domenic merged commit a7f9cdd into WICG:master Jun 25, 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 this pull request may close these issues.

None yet

2 participants