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 import maps registration priorities and error events in-order #143

Merged
merged 10 commits into from
Jul 16, 2019

Conversation

hiroshige-g
Copy link
Collaborator

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

Previously, import maps are processed when they are fetched, and therefore
the priority of import maps and order of error events are nondeterministic.

This PR introduces a mechanism to process import maps in the order of
prepare-a-script, fixes #114.

To do this, this PR

  • Introduces the script's import map, the import map is ready,
    list of pending import map scripts and their related mechanisms similar
    to the script's script, the script is ready, and
    list of scripts that will execute in order as soon as possible.
  • Replaces pending import maps count with
    list of pending import map scripts
  • Moves fields (e.g. acquiring import maps) to Document
  • Make wait for import maps work on Document directly instead of on
    environment settings object.

Other behavior changes:

  • The check for errors in parsing an import map string now precedes the
    check for scripts elements moved between Documents.
  • Import maps being loaded now delay the load event of Documents.

Preview | Diff

@hiroshige-g
Copy link
Collaborator Author

This PR is blocked by the discussion on #114 (not yet finalized).
This PR implements according to [2] of my comment
#114 (comment).

@joeldenning
Copy link
Contributor

I like Option 2 because it doesn't rely on the "acquiring import maps" boolean. I've expressed some other concerns about Option 2, though, in the issue's comments. Overall I think Option 2 might be best.

@hiroshige-g
Copy link
Collaborator Author

One issue in my mind is freedom of scheduling.

The current PR calls register an import map as early as possible.
For example, when an inline import map is inserted to DOM, the error event for that import map (on parse error) is required to be fired synchronously if there are no preceding pending external import maps.

However, probably register an import map is not so urgent things, and calling it anytime on or before wait for import maps is sufficient.

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.

Introduces the script's import map, the import map is ready,
list of pending import map scripts and their related mechanisms similar
to the script's script, the script is ready, and
list of scripts that will execute in order as soon as possible.

Is it possible to reuse the script's script (maybe renamed "the script's result" or "the script element's result") and "the script is ready" (maybe renamed ("the script element is ready")? It seems like it would fit in fine since everything in step 26 is guarded by the type.

If we were to follow the existing spec structure more closely, I think:

  • The "Append the element to the element’s node document’s list of pending import map scripts." step would move to 26
  • The "When the import map is ready" step would move to step 26.

Do you think such a change would make sense?

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domenic
Copy link
Collaborator

domenic commented Jun 26, 2019

However, probably register an import map is not so urgent things, and calling it anytime on or before wait for import maps is sufficient.

I think it is kind of nice to get the error event sooner rather than later, but if you have a design that you think would be nicer I'm definitely open to it.

@hiroshige-g
Copy link
Collaborator Author

Is it possible to reuse the script's script (maybe renamed "the script's result" or "the script element's result") and "the script is ready" (maybe renamed ("the script element is ready")? It seems like it would fit in fine since everything in step 26 is guarded by the type.
If we were to follow the existing spec structure more closely, I think: ...

Sounds good, but needs more checks.

Probably, if we want to follow the existing spec structure, we can do one step further:

  • Introduce a third subtype of #concept-script, i.e. import map script
  • Make fetch an import map return an import map script
  • the script's script is always a script
  • Merge register an import map into execute a script block

This is what I and @nyaxt (IIUC) intentionally avoided, because the blocking semantics of import maps (i.e. ordering between import maps and other scripts) is governed quite differently from the existing mechanisms around execute a script block. For example, the order between classic/module scripts is controlled by the order of execute a script block calls, controlled by e.g. list of scripts that will execute when the document has finished parsing. On the other hand, the order between an import map and module script fetch are controlled by wait for import maps which is outside the mechanisms around execute a script block.
However, we've realized that we need a mechanism to control the order between multiple import maps, that controls error event order and priority of import maps. This can fit with the existing mechanisms around execute a script block (+ we'll still need the ordering mechanism for import-maps vs. scripts outside this), which might justify introducing the import map script. I missed this aspect mainly because we didn't consider multiple import map merging at early stages of package map design.

@domenic and @nyaxt, WDYT? (I'll anyway draft this tomorrow-ish, to see how it would look like)

@domenic
Copy link
Collaborator

domenic commented Jun 27, 2019

Hmm, interesting. I am OK either way, if you think we should go for that full type of merger, or just treat them more similarly inside prepare-a-script (e.g. by reusingthe script's script).

@hiroshige-g
Copy link
Collaborator Author

Is it possible to reuse the script's script (maybe renamed "the script's result" or "the script element's result") and "the script is ready" (maybe renamed ("the script element is ready")? It seems like it would fit in fine since everything in step 26 is guarded by the type.

  • Make fetch an import map return an import map script

Done.

  • Introduce a third subtype of #concept-script, i.e. import map script
  • the script's script is always a script
  • Merge register an import map into execute a script block

I didn't do these, because I found these don't simplify the spec further, and there are substantial differences between #concept-script and import map script.

@domenic
Copy link
Collaborator

domenic commented Jul 11, 2019

There is a problem with error handling. "Parse an import map" can throw an error. The current draft in this PR doesn't handle the error at all. I think it probably assumes that "parse an import map" would return null for parsing failures. But we shouldn't just convert any thrown errors into null as that loses the distinction between a parse error (window.onerror) and a fetch error (script.onerror).

I think the correct behavior would be to treat this similar to what we do for classic/module scripts, but simpler:

  • Parsing errors should get stored as an "error to rethrow"
  • "Prepare a script" should send such errors to window.onerror. (For classic scripts this is done in run a classic script 8.3.1. For our case the counterpart algorithm is "register an import map".)
  • This probably means we need a structure that can hold an import map + possible error to rethrow, and that "the script's script" (which I just renamed "the script's result") should hold this structure. Naming this will be kind of annoying. One idea is "import map parse result" but maybe you have a better idea.

@hiroshige-g
Copy link
Collaborator Author

Good catch. Then import map parse result would have:

  • A settings object (I noticed the plumbing of settings object is also missing)
  • An import map
  • A parse error or an error to rethrow

This is more similar to a script, but still I'll keep this non-script.
(Blink implementation might have ImportMapPendingScript as a third subclass of PendingScript, but not ImportMapScript as a subclass of Script)

So far I don't have ideas for better names; I'll draft commits using import map parse result (and we can easily rename once we come up with a better name later).

@domenic
Copy link
Collaborator

domenic commented Jul 11, 2019

What will the settings object be used for?

@domenic domenic added this to the MVP milestone Jul 11, 2019
@hiroshige-g
Copy link
Collaborator Author

What will the settings object be used for?

For checking whether the import map is moved between Documents.

@hiroshige-g
Copy link
Collaborator Author

Hmm, firing window.onerror increases the motivation to make import map parse result a script and merge the caller of register an import map into execute a script block for consistency, and also makes me wonder about some issues (see register an import map in the latest commit).
I feel this is making register an import map too script-ish only just to fire error events...

(I'll think more about this later though)

spec.bs Outdated
1. Increment/decrement <a spec="html">ignore-destructive-writes counter</a> of |element|'s [=node document=].
1. <a spec="html">Check if we can run script</a> with |settings object|. If this returns "`do not run`" then return.
1. <a spec="html">Prepare to run script</a> given |settings object|.
1. <a spec="html">Clean up after running script</a> given |settings object|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am pretty sure the only potentially observable thing here would be the "check if we can run script" step.

I guess that would be observable because it would make inserting import maps into non-fully active documents a no-op, which you could then observe by making the document fully active again? (Which I think can only happen with bfcache.) Wow, what an edge case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll remove these for now.

spec.bs Outdated
1. <a spec="html">Clean up after running script</a> given |settings object|.
</div>
1. <a spec="html">Report the exception</a> given |import map parse result|'s [=import map parse result/error to rethrow=].
<p class="issue">There are no relevant [=script=], because [=import map parse result=] isn't a [=script=]. Perhaps <a href="https://github.com/whatwg/html/issues/958">whatwg/html#958</a> might resolve this issue.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, this is the best we can do until whatwg/html#958 is solved.

spec.bs Outdated Show resolved Hide resolved
@domenic
Copy link
Collaborator

domenic commented Jul 11, 2019

Yeah, I am torn. The "script" concept gets stretched pretty far, especially with new stuff like JSON module scripts.

If it helps, I made the following matrix:

Classic JSON Module JS Module Import map
Integrates with <script> Y * Y Y
Inner data or parse error Y Y Y Y
Fetch options/base URL for dependencies Y Y
Can be returned by fetch a single module script Y Y

* = I think this is currently allowed but there is talk of disallowing it? Can't find the issue

@domenic domenic merged commit 3c36105 into WICG:master Jul 16, 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.

Order/Priority of multiple import maps
3 participants