-
Notifications
You must be signed in to change notification settings - Fork 28
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
Use coincident
if crossOriginIsolated
, comlink
otherwise
#126
Conversation
Maybe we need to be using |
Right, I'll have a closer look. |
This seems to be caused by It seems to be working fine when serving the site with right headers, for example with: I thought |
Switching to coincident-widget-error.webm
Maybe we need to clearly separate the case when cc @martinRenou in case you have some input on this |
That's unfortunate, but if it's the only solution than let's go for it. coincident is really taking the burden of implementing a proper communication protocol using SharedArrayBuffers, so it's really useful to us in that case. That's unfortunate it cannot work in an asynchronous fashion though, closer to the comlink approach. |
Thanks for looking into it by the way 👍🏽 I'm swamped with other projects. We should apply similar changes to jupyterlite/xeus once we have a proper solution. |
Yeah... As realizing the merit of the SAB is getting more and more tenuous,
it looks like it needs to be entirely opt-in.
…On Wed, Jul 17, 2024, 03:42 martinRenou ***@***.***> wrote:
Thanks for looking into it by the way 👍🏽 I'm swamped with other
projects. We should apply similar changes to jupyterlite/xeus once we have
a proper solution.
—
Reply to this email directly, view it on GitHub
<#126 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALCRHRBWNJL2KS5OEY7I3ZMYVBBAVCNFSM6AAAAABKYZQMG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZSG43DENZXGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Looks like we could keep the current logic of going over the If the |
I'll push a first draft towards that direction and will clean things up a bit afterwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could technically keep both entrypoints in a single file. But having a separate file for coincident
and comlink
may be easier to reason about.
postMessage
to coincident
coincident
when crossOriginIsolated
, comlink
otherwise
coincident
when crossOriginIsolated
, comlink
otherwisecoincident
if crossOriginIsolated
, comlink
otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thanks for the review! |
This PR caused the breakage of interact, see: |
Looking into jupyterlite/jupyterlite#1424.
coincident
uses a SharedArrayBuffer here: https://github.com/WebReflection/coincident/blob/01cc8a1e90abb360deb9eabd7773ef4355fcbc3d/esm/index.js#L82However in environments that are not
crossOriginIsolated
, usingscoincident
on the worker side leads to the following issues:So this PR takes the following approach:
coincident
on both the main thread and the worker if `crossOriginIsolated, to fixUncaught TypeError: i is undefined
jupyterlite#1424 / Uncaught TypeError happened in Pydodide intialization #127comlink
otherwise, this time on both the main thread and the worker tooFixes #127