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

Proposal: Add "extendWorker" extension point #3923

Merged
merged 3 commits into from Oct 27, 2023

Conversation

garrettjstevens
Copy link
Collaborator

I'll explain the reasoning behind this extension point, and I'm certainly open to other ideas of ways to solve it if needed.

In Apollo, data about assemblies, features, sequences, etc. are stored in the session, and the Apollo display doesn't use web workers since it directly observes that store. For things like the regular JBrowse sequence track, though, it does use a web worker, and so can't read from the Apollo store in the session. We've gotten around this by creating a "FromConfigSequenceAdapter" when opening local GFF3s, but that duplicates a lot of data.

This extension point allows us to extend workers, which I've then used in Apollo to add an extra event listener in the worker. This allows the worker to query the main thread for data, which goes outside the normal RPC architecture.

If we ever do a server-side RPC driver, this extension point could also potentially be used to add web socket connections to the server.

@garrettjstevens garrettjstevens self-assigned this Sep 12, 2023
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 12, 2023
@garrettjstevens garrettjstevens changed the title Proposale: Add "extendWorker" extension point Proposal: Add "extendWorker" extension point Sep 12, 2023
@garrettjstevens
Copy link
Collaborator Author

Note from meeting: try using the same pattern the status callback uses in libRpc to add this communication channel.

@garrettjstevens
Copy link
Collaborator Author

This has been updated so that the extension point is evaluated during the "call" step of the base RPC driver. This enables the same behavior that is used in the WebWorkerRpcDriver where worker.on and worker.off can be accessed and set during the RPC call.

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #3923 (56934be) into main (daf93bb) will increase coverage by 0.03%.
Report is 10 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3923      +/-   ##
==========================================
+ Coverage   64.35%   64.38%   +0.03%     
==========================================
  Files        1004     1004              
  Lines       29862    29863       +1     
  Branches     7160     7160              
==========================================
+ Hits        19217    19227      +10     
+ Misses      10482    10471      -11     
- Partials      163      165       +2     
Files Changed Coverage Δ
packages/core/rpc/BaseRpcDriver.ts 83.33% <100.00%> (+0.16%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@garrettjstevens garrettjstevens marked this pull request as ready for review October 4, 2023 16:54
@garrettjstevens garrettjstevens added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Oct 27, 2023
@cmdcolin
Copy link
Collaborator

regarding the need for the web worker to request data from the main thread, it is just a little bit outside the way we've done things so far so it's hard to evaluate from an architecture standpoint, but technically it is probably fine. an alternative architecture could involve the main thread to "sending serialized data to the worker via an rpc call", and then the worker deals with it, but if it seems like this particular architecture is better, we can go ahead with this.

@cmdcolin
Copy link
Collaborator

i'll just go ahead with merger as it is fine codewise for the extension point

@cmdcolin cmdcolin merged commit 9ee1534 into main Oct 27, 2023
11 checks passed
@cmdcolin cmdcolin deleted the add_extendworker_extension_point branch October 27, 2023 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants