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

Add worker modules #608

Merged
merged 1 commit into from Feb 12, 2016

Conversation

5 participants
@domenic
Copy link
Member

domenic commented Feb 2, 2016

/cc @ajklein @DigiTec @Constellation @jonco3

Builds on top of the editorial #607 (the first commit), and split into two commits for easier reviewing. This is a pretty simple spec, with most of the complication being in threading the CORS-awareness of modules through workers.

These commits also make it very easy to add CORS support to non-module workers, if people are interested in that while they are mucking around with this area of their implementation. If so let me know and I can tack on another commit doing so.

/cc @jungkees since you are probably going to want to do some of this for service workers too.

@domenic domenic force-pushed the worker-module branch from fc7510a to d5c09d0 Feb 2, 2016

source Outdated
};

enum <dfn>WorkerType</dfn> { "classic", "module" };
enum <dfn>WorkerCrossOriginMode</dfn> { "anonymous", "use-credentials" };

This comment has been minimized.

@annevk

annevk Feb 2, 2016

Member

When you pass "omit" here it's not a crossOrigin mode. How about we name this WorkerModuleCredentialsMode and moduleCredentials as member (or just credentials as all new features will require the opt-in anyway)?

This comment has been minimized.

@domenic

domenic Feb 2, 2016

Member

You don't pass omit. It mirrors the crossOrigin IDL attribute/crossorigin="" content attribute: missing (null) = omit, anonymous = same-origin, use-credentials = include.

We could use fetch()'s RequestCredentials enum, but I thought it'd be better to stick with the terms and API from HTML. WDYT?

This comment has been minimized.

@annevk

annevk Feb 2, 2016

Member

It seemed a little ugly to me to reuse that for a new API. Nullable enum still feels wrong. Workers happen to be in HTML, but that's it.

This comment has been minimized.

@domenic

domenic Feb 2, 2016

Member

One advantage of the existing setup is that we can use null to mean different defaults if we extend CORS to classic workers (there null -> same-origin, for back-compat). I guess we could do that with the new scheme though too; we just use RequestCredentials? credentials = null and determine what null means based on classic vs. module.

I'm happy to switch this to RequestCredentials credentials = "omit" for now if you think that's best for new APIs.

This comment has been minimized.

@domenic

domenic Feb 2, 2016

Member

Fixed in a separate commit.

@domenic domenic force-pushed the worker-module branch from 7ab1e80 to 85898c9 Feb 2, 2016

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Feb 3, 2016

I was worried we would not be setting origin and referrer correctly but since we have the correct environment settings object I think that will all work fine.

I was thinking that if we do not change classic workers we might entice folks to upgrade to module workers sooner since they can be hosted on a CDN.

For service workers the main service worker script is required to be same-origin. There are no restrictions on importScripts() however. So perhaps some kind of flag on "fetch a module script tree" that forces the initial fetch to be same-origin is needed.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 4, 2016

@ajklein @DigiTec @Constellation @jonco3 can I get some comments on this? Is this something we should add to the spec, and you are interested in implementing as you work on modules? Would really be great to get everyone's opinions here.

@ajklein

This comment has been minimized.

Copy link
Collaborator

ajklein commented Feb 11, 2016

There's no doubt that support for modules in Workers is something I, as an implementer (in Chromium/Blink/V8), want to make available on the web. I think most of the interesting bits of this proposal, though, have to do with the API, not with implementation concerns.

Workers are less constrained than script tags, since they don't have to interact with the HTML parser and the page-loading system. This might make us wish for a constructor that defaults to module mode, rather than having to pass a second argument to get the module behavior (of course this comes down to a naming problem).

I'm also not sure that we want to expose importScripts() on the global scope of module workers, as that could well be confusing to future programmers.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 11, 2016

Thanks for commenting @ajklein!

My intention was for workers, shared workers, and service modules all to be module-ifyiable. If we introduce a new worker constructor, and a corresponding new type of WorkerGlobalScope, I don't think we accomplish this---or at least we cause a small combinatorial explosion as we add (Shared|Service)Worker(Module)(GlobalScope). I also don't think the ergonomic benefits of new WorkerModule(s) over new Worker(s, { type: "module" }) are strong.

I'd be up for disabling importScripts inside module workers though. I thought it would be good to allow it so that people can continue to use things like the service worker cache polyfill (which is not authored as a module) but maybe that's short-term thinking.

@DigiTec

This comment has been minimized.

Copy link

DigiTec commented Feb 11, 2016

FYI this has our interest ;-) I've read the diffs and I don't see anything that would pose difficulties for our implementation. I think the only open question is whether or not we should continue to allow importScripts.

Thinking through use cases. I have a worker and I want to import another module, but I don't have access to imperative module loader APIs yet. In the HTML spec, you can start a new module root with script type="module" but workers don't have the same option unless we update importScripts to allow a module type as well. This thinking is also likely short-term as more pieces of modules land.

So I'd be okay removing importScripts in the type="module" worker in order to simplify things. It does mean that worker modules are effectively 100% declarative until module loader APIs come along unless i'm missing something else.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 11, 2016

Awesome, thanks @DigiTec!

It sounds like there's a collective weak leaning toward disabling importScripts for modules, at least for now. I'll work on updating things in that direction (but keep the commit around in case @Constellation or @jonco3 feel strongly we should keep importScripts). Note that I'll just make importScripts throw a TypeError immediately in a module worker, instead of trying to create some new type of WorkerGlobalScope.

@annevk, I'll ping this thread again when that's ready for editor review. Regarding

For service workers the main service worker script is required to be same-origin. There are no restrictions on importScripts() however. So perhaps some kind of flag on "fetch a module script tree" that forces the initial fetch to be same-origin is needed.

I think we should leave that for the time when service workers does the integration work, so we can have a discussion on the service worker repo where you educate me as to why it has to be same-origin even with CORS :).

@jonco3

This comment has been minimized.

Copy link

jonco3 commented Feb 11, 2016

This looks good to me.

I agree with @ajklein about importScripts. I don't think this really make sense for module workers.

@ajklein

This comment has been minimized.

Copy link
Collaborator

ajklein commented Feb 11, 2016

@domenic I'll buy that given the wealth of different worker constructors an options arg is probably the lightest way to go. Thanks for expanding on that.

@domenic domenic force-pushed the worker-module branch from 85898c9 to 1e9c63c Feb 11, 2016

@domenic domenic force-pushed the worker-module branch from 1e9c63c to c265cc7 Feb 11, 2016

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 11, 2016

@annevk ready for review! importScripts now throws a TypeError in module workers. (Which, for the record, turns out to be a better name than "worker modules", because "worker classics" doesn't work.)

@domenic domenic force-pushed the worker-module branch from c265cc7 to 780cdd4 Feb 11, 2016

source Outdated
@@ -93430,6 +93505,11 @@ function showLogout() {

<pre>var worker = new Worker('helper.js');</pre>

<p>If you want your worker script to be interpreted as a <span>module script</span> instead of
the default <span>classic script</span>, you can use a slightly different signature:</p>

This comment has been minimized.

@annevk

annevk Feb 12, 2016

Member

s/can/need to/

source Outdated
<p><span data-x="run a classic script">Run the classic script</span> <var>script</var>.</p>
<p>Perform either the <span>run a classic script</span> or <span>run a module script</span>
algorithms on <var>script</var>, depending on whether it is a <span>classic script</span> or
<span>module script</span>.</p>

This comment has been minimized.

@annevk

annevk Feb 12, 2016

Member

This doesn't clearly tie them together other than through intuition that "classic script" is coupled with "run a classic script". It's also not entirely clear to me what "it" is referring to. Something like "Run a classic script" if /x/ is a "classic" script", and "run a module script" otherwise." would be better I think.

source Outdated

dictionary <dfn>WorkerOptions</dfn> {
<span>WorkerType</span> type = "classic";
<span>RequestCredentials</span> credentials = "omit";

This comment has been minimized.

@annevk

annevk Feb 12, 2016

Member

Perhaps add an IDL comment here that "credentials" is only used if WorkerType is not "classic"?

@@ -94528,6 +94617,8 @@ interface <dfn>SharedWorker</dfn> : <span>EventTarget</span> {
<p>To <dfn>import scripts into worker global scope</dfn>, the user agent must run the following steps:</p>

<ol>
<li><p>If this <code>WorkerGlobalScope</code> object's <span data-x="concept-WorkerGlobalScope-type">type</span> is
"<code data-x="">module</code>", throw a <code>TypeError</code> and abort these steps.</p></li>

This comment has been minimized.

@annevk

annevk Feb 12, 2016

Member

I have two thoughts here:

  1. It would be nicer if the method was simply not exposed.
  2. The way we designed this setup for service workers was that it could overwrite certain algorithms here. Should we not be overwriting the "validate the state" algorithm? I do think that would be less readable so perhaps it's simply a manner of the current setup not being ideal.

This comment has been minimized.

@domenic

domenic Feb 12, 2016

Member
  1. That would require creating a new type of WorkerGlobalScope (and thus creating new DedicatedWorkerGlobalScope/SharedWorkerGlobalScope/ServiceWorkerGlobalScope derivatives). I'd really like to avoid that. They're still workers.
  2. I thought about that, but then I said: does any caller ever want to customize this behavior? E.g. should we allow service workers to do importScripts in modules, via a different validation step? And the answer is no, I think. We want to enforce at the HTML spec level that this never works.

This comment has been minimized.

@annevk

annevk Feb 12, 2016

Member

Satisfactory.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Feb 12, 2016

@annevk pushed a fixup1 commit addressing your feedback.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Feb 12, 2016

LGTM then. The reason service workers require same-origin by the way is to ensure that the domain itself controls the service worker. If the service worker could be controlled by another domain that would make XSS even uglier since the domain might never find out it lost a bunch of users to that other domain.

@domenic domenic force-pushed the worker-module branch from d66cbf4 to e3a5bb7 Feb 12, 2016

Add module workers
Closes #550. The Worker and SharedWorker constructors now get an options
object, which can be specified as { type: "module", credentials }, where
credentials specifies the credentials mode used for the initial fetch
and for subsequent imports. (This only applies to module workers; in
the future we could extend it to classic workers if we wished.)

importScripts will always throw a TypeError in modules, per discussion
in the pull request.

The module fetching and execution machinery is entirely reused from that
for <script type="module">. This also refactors the machinery for
fetching a classic worker script out into its own sub-algorithm of the
"Fetching scripts" section.

@domenic domenic merged commit e3a5bb7 into master Feb 12, 2016

@annevk annevk deleted the worker-module branch Feb 15, 2016

dstockwell pushed a commit to dstockwell/chromium that referenced this pull request Feb 18, 2016

Add a basic version of Worklet#import.
Worklets are now spec'd to use the 'module' loading infrastructure instead of the classic scripts.
'module' loading is still getting nailed down, so at the moment just load a 'classic' script.

https://html.spec.whatwg.org/multipage/scripting.html#concept-script-type

There is a large TODO to change this over when 'module' loading becomes mature.

Workers will also (probably) need to support 'module' script loading in the future,
(see: whatwg/html#608 )
so any changes to script controller / script loader will benefit both.

Worklet now inherits from ActiveDOMObject due to script loading.

BUG=567358

Review URL: https://codereview.chromium.org/1684303002

Cr-Commit-Position: refs/heads/master@{#376271}

@domenic domenic added topic: script and removed topic: script labels Sep 1, 2017

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