-
Notifications
You must be signed in to change notification settings - Fork 313
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 arguments to "fetch a classic worker script" #1012
Conversation
This follows the analogous fix in whatwg/html#2077 for HTML.
<dl> | ||
<dt><em>"<code>classic</code>"</em></dt> | ||
<dd><p><a>Fetch a classic worker script</a> given <var>job</var>’s <a lt="URL serializer">serialized</a> <a href="#dfn-job-script-url">script url</a>, <var>job</var>’s <a href="#dfn-job-client">client</a>, and "<code>serviceworker</code>".</p></dd> | ||
<dd><p><a>Fetch a classic worker script</a> given <var>job</var>’s <a lt="URL serializer">serialized</a> <a href="#dfn-job-script-url">script url</a>, <var>job</var>’s <a href="#dfn-job-client">client</a>, "<code>serviceworker</code>", and the to-be-created <a>environment settings object</a> for this service worker.</p></dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An environment settings object for each service worker execution changes. Run Service Worker step 7 creates one before run the script in step 14. So, I guess we should set the script's settings object slot to the created environment settings object in Run Service Worker. And just pass null to Fetch a classic worker script algorithm's script settings object param here, if that makes sense. For that, additional guards would be needed in create a classic script I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to fix this in a separate issue. It seems really hard to me to fix. You can't just mutate a script's settings object slot like that; the settings object determines how the script is parsed and some other important things, which happens during fetching. You can't pass null in that case as otherwise it's impossible to parse the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Having looked at those fetch/run worker script algorithms, in the case of module script, the parsing is all done in creating a module script which is called during the fetching phase. The problem we have with SW script execution model is that fetching and running execute in separate places with a different lifecycle from that of other workers. I don't think I fully understand the module script execution model, but would it make sense to do something like:
- SW: Create a (concrete but initialized with minimal things) environment settings object before Update step 7 and give it to fetch a module worker script graph.
- HTML: This above steps compose the module map as expected and it's stored in the script asynchronously returned as a result of the fetch.
- SW: That script is stored in the service worker's script resource slot.
- SW: When Run Service Worker is invoked, it's given the service worker and the script in the script resource slot. In this algorithm, when a new environment settings object is created, copy necessary states from the given script's settings object. Or even just reuse this script's settings object as-is to run the script? (Not sure this scenario makes sense. Can a realm created during the fetching phase can be reused in all the subsequent Run Service Worker script executions?)
Anyway, agreed that we'd better fix it in a separate issue. I'll merge this to be aligned with HTML changes in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining issue filed: #1013.
<dt><em>"<code>module</code>"</em></dt> | ||
<dd><p><a>Fetch a module worker script graph</a> given <var>job</var>’s <a lt="URL serializer">serialized</a> <a href="#dfn-job-script-url">script url</a>, <var>job</var>’s <a href="#dfn-job-client">client</a>, "<code>serviceworker</code>", "<code>omit</code>", and the to-be-created <a>environment settings object</a> for this service worker.</p></dd> <!-- TODO: reorganize algorithm so that the worker environment is created before fetching happens --> | ||
<dd><p><a>Fetch a module worker script graph</a> given <var>job</var>’s <a lt="URL serializer">serialized</a> <a href="#dfn-job-script-url">script url</a>, <var>job</var>’s <a href="#dfn-job-client">client</a>, "<code>serviceworker</code>", "<code>omit</code>", and the to-be-created <a>environment settings object</a> for this service worker.</p></dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
This follows the analogous fix in whatwg/html#2077 for HTML.
Ideally this should be merged after the HTML pull request, but in practice it should be fine either way, as the HTML PR is pretty straightforward and will likely be merged forthwith.