-
Notifications
You must be signed in to change notification settings - Fork 129
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
Proposal: Sync in worker #1041
base: master
Are you sure you want to change the base?
Proposal: Sync in worker #1041
Conversation
Thanks for this proposal, some early thoughts:
|
I just saw you mentioned the worker being owned by the service worker. That has the same limitation that hydrogen wouldn't be able to run without a worker though. A SharedWorker seems like what we want, but support is lacking on mobile platforms. Perhaps that's good enough though, use a service worker in production, and a SharedWorker during development time. We should of course still be able to run sync within the page and disable cross-tab syncing. |
A good first step would be to list things of things that would need their updates passed over the message channel between the worker and the page:
|
Thanks for looking into this proposal @bwindels! Agree, we would be making the changes behind a feature flag, which would allows us to spread the implementation across multiple PRs, and iterate on the feature until we're confident that it's stable. When the feature flag is disabled, or the environment doesn't allow for running sync in a worker (e.g. no support for Web Workers API), we would fallback to the non-worker sync, exactly as is today. If necessary, we could also make it possible to disable sync-in-worker through runtime config, or even at the build level. I agree Hydrogen must continue to be functional without a service worker. I will look further into this topic, and into the "after sync" step and will update the document accordingly. I've updated the PR description with a TODO section. |
@bwindels Is the opposite scenario also needed, i.e. passing updates to the worker when the
So if mutations to the |
hmm, you could probably get away with relying on remote echo, e.g. the /sync api reflecting a change you made like sending a message, creating a room, ... there will be a delay for the server roundtrip but that should be ok? |
Relying on sync to propagate the changes makes sense, server roundtrip should be fine indeed. |
1e3905c
to
6db66e8
Compare
I've also created #1045 to track the feature, which would also serve as the epic for this endeavour. I've started extracting actionables from this proposal into a TODO section on that issue. Let me know if there's anything in that TODO that seems off, or if you have any other feedback. In the meantime, I'm looking into the other topics in the |
|
@bwindels I've clarified the service worker vs shared worker topic in the doc. In short, there's one const sessionId = "foo";
const worker = new SharedWorker(new URL('./sync-worker.js'), {
name: `sync-worker-${sessionId}`,
}) This code can run in multiple windows/tabs/iframes. The browser spawns a worker the first time one is created with the given |
Just saw that workers of type es module are coming to firefox! This means we may not need a build-step to convert |
Addresses #1045
This is a proposal for running sync in a worker, so that it would be possible to have the same session open in multiple tabs/windows/iframes.
Rendered
TODO
SharedWorker
Session
in the main thread need to be communicated to the workerBroadcastChannel
.localStorage
inStorage
, sincelocalStorage
is not available in workers