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

<script type=webundle> should fire a load (or error) event? #697

Open
hayatoito opened this issue Oct 26, 2021 · 15 comments
Open

<script type=webundle> should fire a load (or error) event? #697

hayatoito opened this issue Oct 26, 2021 · 15 comments

Comments

@hayatoito
Copy link
Collaborator

hayatoito commented Oct 26, 2021

The main issue is #670.

It's unclear whether <script type=webbundle> should fire a load (or error) event or not.
And if they should be fired, when should <script type=webbundle> fire them?

  • When its inline JSON is successfully parsed?
  • When the bundle specified in source is loaded?
  • Or any other timings?

For references, the other <script type=>-ish elements' behaviors are:

We might want to align the event semantics of <script type=webbundle> with Bundle Preloading so that we can converge them in the future.

hayatoito added a commit to hayatoito/webpackage that referenced this issue Oct 26, 2021
hayatoito added a commit that referenced this issue Oct 26, 2021
… event (#696)

* Clarify that <script type=webbundle> doesn't fire a load event

issue #670.

* Mention an error event and issue #697
@hayatoito
Copy link
Collaborator Author

cc: @felipeerias

@hayatoito
Copy link
Collaborator Author

As a workaround, until we can have a consensus, users can try to fetch a (small) subresource immediately after inserting the script element into a document.

For example:

const script = document.createElement("script");
script.type = "webbundle";
script.textContext = JSON.stringify({ .... });
document.body.append(script);
fetch(a_tiny_subresource_in_bundle).then((_) => {
  // Alternative of a load event listener of a script element
}).catch((error) => {
  // Alternative of an error event listener of a script element
})

I'm not 100% sure whether this workaround is always applicable, however, this is fine in most cases.

Any ideas or opinions are welcome!

@cjtenny
Copy link

cjtenny commented Oct 26, 2021

One thing I'm a little confused about, what do you mean by 'it fetches a bundle many times in its lifetime'? Our intention was that each bundlepreload section would result in a single request with a Bundle-Preload header; there would be no additional Bundle-Preload requests from the script element. After the page has sent a Bundle-Preload request, regular, non-<script type=bundlepreload> requests for the specified resources will then be served by the browser from the received bundle - blocking until it is available.

To review:

  • Bundlepreload sections are intended as a prefetch, specifying that future requests to those resources will be served by the browser from the bundle response.
  • A bundle response might not contain all the requested subresources, or might contain 404s, 500s, etc for some of them.
  • A bundle response might contain additional subresources

In my mind, then, it might make sense to send:

  • an error event if the JSON is not valid, as no request will be sent
  • a load event for when the bundle response conforming to the header is fully received and processed, as for a JS script with an external source
  • an error event if the received response doesn't have all the resources specified in the Bundle-Preload header
  • an error event if the received response has additional resources specified in the Bundle-Preload header, allowing the page to detect that the server may not have understood the request, served the entire bundle as a file, etc.

The load event would be separate from the load events sent by later references to the bundled resources, which would not be sent from the script element.

What do you think? Does that match the behavior of other script type elements? I'm not familiar with speculation rules, but I'll take a look.

@cjtenny
Copy link

cjtenny commented Oct 26, 2021

As a workaround, until we can have a consensus, users can try to fetch a (small) subresource immediately after inserting the script element into a document.
...
I'm not 100% sure whether this workaround is always applicable, however, this is fine in most cases.

If resources can be served from the bundled response before the whole bundle has been received, then it would be possible for this to fire a load event but the bundle itself to fire an error event - I don't know if that's good or bad. Otherwise, I think it would be a perfect workaround. Perhaps another subresource with less overhead than a script could be a little faster and more accurate for timing, but it probably doesn't matter much.

@hiroshige-g
Copy link
Collaborator

Basically, <script> load and error events indicate successful/failing resource loading, while parse errors go to Window's error events (e.g. window.onerror).
Inline <script type=webbundle> can trigger resource loading, so it might make sense to fire load/error events based on whether the loading of webbundle files was successful.
Inline <script type=importmap> doesn't fire load/error events, because inline import maps itself doesn't trigger resource loading.
Similar discussion for firing load events for inline <script type=module> is at whatwg/html#6421.

@cjtenny
Copy link

cjtenny commented Oct 27, 2021

Got it, thank you for clarifying! It seems then like the first event I proposed should go to window.onerror, and the other 3 make sense for the script events. Does that sound reasonable? Are there any implications to that which could cause problems?

@hayatoito
Copy link
Collaborator Author

Thanks for the comments! It seems what we want to do is:

  • JSON parsing error => windown.onerror
  • bundle loading finished successfully => script.onload
  • bundle loading failed => script.onerror

This sounds reasonable to me at first glance. At least, this would be consistent with the direction at whatwg/html#6421.

@hiroshige-g
If there are any implications which could cause problems, please let us know that.

@cjtenny
Copy link

cjtenny commented Oct 27, 2021

Yeah, and I suppose defining "bundle loading failed" sub-cases is still a bit up in the air / might depend on bundle-preloading semantics. We can explore further what that is separately from the question of whether to fire the event, what do you think?

@hayatoito
Copy link
Collaborator Author

hayatoito commented Oct 27, 2021

Yeah, and I suppose defining "bundle loading failed" sub-cases is still a bit up in the air / might depend on bundle-preloading semantics. We can explore further what that is separately from the question of whether to fire the event, what do you think?

Agreed. I guess defining "bundle loading failed" might be tricky on the bundle-preloading case. So we can explore that further as another issue.

@hiroshige-g
Copy link
Collaborator

What should occur when:

  1. The first <script type=webbundle> was added with source = foo.wbn.
  2. The loading of foo.wbn succeeded or failed.
  3. Then the first element was replaced with the second <script type=webbundle> with source = foo.wbn.

IIUC Step 3 doesn't trigger new network request to foo.wbn. (This is intentional for successful cases. Also good for failed cases?)
Should the load/error event be fired at Step 3 for the second element?

@hayatoito
Copy link
Collaborator Author

A good question.

For those who are not familiar with the context, see the requirement 2 of <script type=webbundle> design doc, which might be helpful to understand the context more.

IIUC Step 3 doesn't trigger new network request to foo.wbn. (This is intentional for successful cases. Also good for failed cases?)

Yeah, step 3 doesn't trigger new network request. We've not decided the behavior for failed cases. Let's assume step 3 doesn't trigger new network request for a failed case too.

Given this assumption,

Should the load/error event be fired at Step 3 for the second element?

I don't have a strong preference. The behaviors of the Step3 would be one of the followings?

1st one fired a load 1st one fired an error
A fire a load fire an error
B fire nothing fire nothing
C fire a load fire nothing
D fire nothing fire an error

Either A or B looks consistent to me. I am slightly in favor of A if we consider "re-using" is just an internal optimization and want to minimize the visible side effect.

@irori
Copy link
Collaborator

irori commented Oct 29, 2021

+1 to A.

I don't like C and D. From the user's point of view, it is difficult to use if it's not guaranteed that either load or error will be fired.

@hiroshige-g
Copy link
Collaborator

+1 to A. It is consistent with other cases where we fire load (and error) events without any fetch requests due to caching in the HTML spec layer:

Also it would keep the invariant of one load or error event for every <script type=webbundle>, which might be easier to understand for users and web developers.

We've not decided the behavior for failed cases.

Yeah, whether we reuse the failed cases or not is a separate issue to be determined.

@cjtenny
Copy link

cjtenny commented Nov 10, 2021

What should occur when:

1. The first `<script type=webbundle>` was added with source = `foo.wbn`.

2. The loading of `foo.wbn` succeeded or failed.

3. Then the first element was replaced with the second `<script type=webbundle>` with source = `foo.wbn`.

IIUC Step 3 doesn't trigger new network request to foo.wbn. (This is intentional for successful cases. Also good for failed cases?) Should the load/error event be fired at Step 3 for the second element?

For bundle-preloading, step 3 depends on the contents of the element. If the resources specification contains new resources, a new request would be issued and a new load event should be fired. For consistency, I'm uncertain whether it'd be better to fire an event or not if no new resources are requested.

Sorry for the delay, have been busy with other things - will be around more starting in ~two more weeks. @felipeerias can clarify further while I'm out!

@hiroshige-g
Copy link
Collaborator

My spec PR #721 fires load/error events for every <script type=webbundle>, which is consistent with the comments here and corresponds to A in #697 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants