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

Sync worker: kickoff #1056

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

psrpinto
Copy link
Contributor

@psrpinto psrpinto commented Mar 9, 2023

First step to address #1045.
Please see #1041 for context and motivation on these changes.

This PR serves as the kickoff for having sync running in a worker. With this PR, the sync worker is spawned if a newly-added feature flag is enabled, but won't do anything yet. Follow-up PRs will implement the worker.

I would suggest commit-by-commit review, as I made sure to structure the commits in an iterative way, with context for the change provided in the commit message.

Merging this should have no user-facing changes, unless the user enables the feature flag (which they are advised not to do).

Screen capture

Screenshot 2023-03-09 at 12 03 26

Summary of changes

  1. Create a SameSessionInMultipleTabs feature flag
  2. Make it possible to have multiple implementations of Sync, so we can have sync running in a worker, while making it transparent to Client.
  3. When feature flag is enabled, and browser supports running sync in a worker, Client gets an instance of a SyncProxy, which has the same public API as Sync, but offloads sync to a worker.
  4. Changes to service worker build script so it doesn't fail due to sync-worker.js.

Next steps

  1. Enable Typescript checks on sync-worker.ts. This is not trivial so we'll address it in a separate PR.

So that features are available through the Platform. I think logically it makes sense to have the Platform load the features, as where features are coming from depends on the Platform. On Web they come from localStorage, on other platforms they might come from elsewhere.

This will allow us to inject the FeatureSet into a SyncFactory class in next commits.
When sync is running in a worker, the start() method will need to be async, as we will need to send messages to the worker to start the sync.
I think it makes sense to not have the logic of whether sync runs in a worker in Client, as that is platform-specific, so that logic should not be in the matrix layer.
Will proxy calls to a worker. Doesn't do anything yet.
... and the browser supports SharedWorkers.
Doesn't do anything yet.
Without this the build fails.
@psrpinto psrpinto marked this pull request as ready for review March 9, 2023 15:11
Will address this in a follow-up PR as it's not trivial to fix.
@psrpinto psrpinto changed the title Sync worker kickoff Sync worker: kickoff Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant