Implement Service Worker API #3246
Conversation
| import { Clients, kAddClient } from './clients.js' | ||
| import { ServiceWorker } from './service-worker.js' | ||
| import { Client } from './client.js' | ||
| import { CacheStorage } from '../cache/cachestorage.js' |
There was a problem hiding this comment.
TIL Undici has its own CacheStorage implementation. I'm reusing it for the ServiceWorkerGlobalScope as-is.
| // Run the worker script within the controller global scope. | ||
| const script = new vm.Script(content) | ||
|
|
||
| script.runInNewContext({ |
There was a problem hiding this comment.
This is the heart of the implementation: running the actual worker script in a VM inside a worker.js context. See service-worker-container.js for more details on how worker.js is spawned.
| frameType: '???' /** @todo */ | ||
| } | ||
|
|
||
| const worker = new Worker(new URL('./worker.ts', import.meta.url), { |
There was a problem hiding this comment.
The container (the serviceWorker object) spawns a new worker of worker.js to provide scope isolation akin to how a Service Worker operates in the browser. I suggest we keep that isolation for the worker code not to leak into the main thread.
| * | ||
| * @see https://w3c.github.io/ServiceWorker/#navigator-service-worker-getRegistration | ||
| */ | ||
| getRegistration (clientUrl) { |
There was a problem hiding this comment.
Question
Should we abide by the rule that only one Service Worker can control the same scope, like in the browser? Or should we allow an arbitrary number of Service Workers controlling the main thread?
I lean toward the first option, would like to hear your thoughts on this as well.
| @@ -0,0 +1,3 @@ | |||
| import { ServiceWorkerContainer } from './service-worker-container.js' | |||
|
|
|||
| export const serviceWorker = new ServiceWorkerContainer() | |||
There was a problem hiding this comment.
The API is meant to be consumed via the serviceWorker object:
// main.js
import { serviceWorker } from 'undici'
await serviceWorker.register('./my-worker.js')
| import { FetchInterceptor } from '@mswjs/interceptors/fetch' | ||
| import { XMLHttpRequestInterceptor } from '@mswjs/interceptors/XMLHttpRequest' | ||
|
|
||
| export const interceptor = new BatchInterceptor({ |
There was a problem hiding this comment.
Question
Which interception algorithm do you see more fitting?
I was considering between the MockAgent of Undici and Interceptors from MSW. There are reasons to use either.
MockAgentis in-line with Undici but covers only Undici's implementation offetch. I'd expect the Service Worker API to intercept any HTTP requests, just like it does in the browser.- Interceptors intercepts any HTTP requests but is a third-party library. If you are willing to add it as a dependency, I have nothing against using it. Interceptors is also slowly becoming the backbone for request interceptions for other libraries too, with Nock slowly migrating to use it instead. Using it in Undici may solidify it as a well-established request interception API in Node.js.
| interceptors: [ | ||
| new ClientRequestInterceptor(), | ||
| new FetchInterceptor(), | ||
| new XMLHttpRequestInterceptor() |
There was a problem hiding this comment.
Todo
Need to remove this. There are no XHR in Node.js. We won't be tailoring for browser-like environments like JSDOM.
| /** | ||
| * @param {import('./service-worker-container.js').WorkerData} parentData | ||
| */ | ||
| constructor (parentData) { |
There was a problem hiding this comment.
Todo
Implement the registration representation on the global scope. Something similar to how the self.serviceWorker is implemented.
Those will be different objects in the main thread and in the worker thread but connected via messaging.
|
I'd like to improve the spec-compliance because I spent two last days implementing I still expect some internal deviations and perhaps even public deviations (see the PR description), both of which should not disrupt the usage of the API. |
|
@mcollina, I'm curious how much the Service Worker API falls under the direction of Undici. I'm mainly choosing to land this in Undici since I get an impression it's the frontliner for trying out the experimental things in Node.js. Let me know what you think about where this API belongs to. |
|
Can you add an example or (better) a test to use this? |
|
I'm not sure if undici is the right place to implement this |
|
@mcollina, will do. A basic integration test is the next on my list. @KhafraDev, my thinking as well. Can we please agree on that first, since this PR is a manually edited fork of my repo, which is in TypeScript. I'd love to keep maintaining just one implementation. If it doesn't belong in Undici, I will simply make my repo public and publish this on NPM. We can also create a repo under the Node.js org if that's not running ahead of the bus, of course. I leave this to your best judgment, just please let me know. Thanks. |
|
I don't think it's the correct place as well, and also, regarding tests, due it being behind a web standard spec there are already tests to it part of WPT, we should enable that for them |
|
Unless you are giving me a hard pass, I'd ship this as a third-party package, gave it some time, improve it, and then eventually land it into Node.js, or any more suiting repos. @rluvaton, yeah, that was my thinking as well. Just need to do the test orchestration for that. |
|
There are a number of pieces that are missing from node before this can be done, right? You'd need globalThis to extend EventTarget, you'd need to add I'm sure it's possible to get it running without these, but then the api is fundamentally different and at that point I see little reason to implement a w3c spec. Same for the differences you mentioned - if the api isn't suitable to node I see little reason to implement it. Even then, the benefits you mentioned (caching and intercepting) are both already possible to do in node/undici, and a majority of the use cases that the spec directly outlines are irrelevant to node. Could you outline why service workers should be implemented? |
|
Personally I think that having The only question I have is that if we need other things before implementing this here (HTTP caching support, globalThis, etc). As for landing this here, I think it's the right place mostly because we have a better webidl implementation and generically better support for WPTs. I'm going to ping the @nodejs/tsc before we give you an answer. What I want to avoid is to have another situation similar to what happened to node-fetch when we shipped |
@KhafraDev, this is already done, perhaps partially. I'm using VM context to declare my own
While it's possible to achieve caching in Undici, that is a non-standard behavior specific to Undici. What I'm proposing here is a standard API designated for caching to work together with Undici and beyond, as Undici still has a vastly smaller usage compared to @mcollina, thank you. I'd love to have this answered so I can continue working on this. |
|
I personally won't recommend to touch http.request() for anything. Even bugfixes causes so many regression that is playing whack-a-mole. That's the reason undici exists. I would recommend focusing on fetch(). |
What happened to node-fetch? |
As far as I recall the maintainers were relatively frustrated when we shipped (The same happened with other libraries when we shipped them in core). I want to avoid this situation if possible. |
True, but this is also an http library. Everything web related revolves around fetch (except FileReader, which was largely a mistake that is planned on being removed). I can't see the justification for undici to start implementing web workers and service workers. It is also possible to use undici's webidl outside of undici;
undici also has Cache/CacheStorage.
One of the first lines of the spec. We can't have service workers without web workers but we're skipping past the requirements? It would be like implementing fetch without web streams or Blob; there's a reason fetch came into existence once all of the pieces were in-place. |
|
One more thing, please read what the Deno maintainers have to say about service workers, I think it's largely relevant here:
Server environments can already "intercept" requests, as mentioned previously, by using servers. In an environment that does not have this convenience, a service worker may be needed, but node has the tooling to handle this far better than the service workers api provides. |
The original author (bitinn) was helpful and engaged in a lot of discussions and provided valuable feedback, do you mean string formattting by any chance? nodejs/node#43382 |
Two more reason to keep this a third-party package and evolve it over time. I have no time to dive into a full web worker implementation but the package can already provide value. The requirement bar is simply different here, and I have no problem writing a mention in the README of the lib's limitations. Landing it in Node.js, on other hand, is a far more serious commitment.
Read through that, thanks. I share some of those concerns and it's yet another reason to push this to the userland. I don't mind it being incomplete or partially spec-compliant. I also want the Service Worker API to work with any HTTP request clients, just like it does in the browser, not just fetch. Since Interceptors already provide an extensive interception for I am moving this to userland until Node.js voices the need to have the Service Worker API. It will allow the API to evolve faster while also keeping commitment lower on my end, as well as lower the contribution threshold to the project. Thank you everyone for the discussion! |
|
Thank you for your PR even though it was not merged, we know you put your time into it and we appreciate it if you have a repo I would be happy to take a look and give feedback if you want |
|
@rluvaton, thank you for the encouragement! I'd be grateful if you looked at what I have right now. As I am still working on making it specification-compliant, one area I'd love to hear thoughts on is my overall architecture of this API (the usage of worker_threads + VM) and whether it's a good approach in general. Here's the repo: https://github.com/kettanaito/service-worker. The bits I'd love to have reviewed:
Thank you! ❤️ |
Rationale
Implementing the ServiceWorker API in Node.js can enable request interception and caching capabilities. This becomes more and more common as JavaScript frameworks expand onto the server, with concepts like React Server Components. Those frameworks rely on custom patches of globals to implement features like request deduplication and caching, whereas they could use the standard API designed just for that.
The API itself is meant to be W3C specification-compliant to the best of Node.js abilities. Naturally, things like
.openWindow()and other browser-related methods or properties will not be implemented (I advocate for leaving them there for compatibility but skip the implementation as it's usually a side effect to perform an action in the browser).This implementation may also deviate from the specification where appropriate. For example, representing the
client.typeaswindoworframeis irrelevant and confusing in Node.js. Instead, I'd propose to either coerce the parent/child relationship of the main/worker threads ontowindoworworker, or introduce custom client types likemain-threadandworker.Changes
Implements the classes needed for the Service Worker API (please see the "files" for more detailed overview).
Breaking Changes and Deprecations
This change does not impose any breaking changes as this is a new API being added to Undici.
Status
Implementation
Warning
This is a work-in-progress implementation. I would like to get some code review on it before continuing. It still needs tests, for once. Thank you.
Roadmap
ServiceWorkerContainer.getRegistration()ServiceWorkerContainer.getRegistrations()ServiceWorkerGlobalScope.registrationimportScripts()in the worker context (needs discussion).Client.type(needs discussion)Client.frameType(needs discussion)