Automatically patch js.document and js.window in Workers
#1878
Replies: 1 comment 16 replies
-
|
This is a great topic. Thanks @WebReflection for kicking it off. Agree with you that we have a little bit of a dilemma having to choose transparency versus compatibility/usability. All In all, I think we should lean towards making the user experience as pleasant as possible while at the same time leaving an escape hatch. How about we keep the patches as a default behavior and offer a config option to turn it off? This way the default will probably match what most users would need but we still have an option for people to explicitly turn it off and use Pyodide's own interface. |
Beta Was this translation helpful? Give feedback.
-
|
last thoughts from me:
if the config flag is to opt-out, we could indeed just make the patch the default and re-discuss, and re-consider, the config flag in the future, whenever anyone will have issues with us patching globally the |
Beta Was this translation helpful? Give feedback.
-
|
we're here 🥳 #1880 |
Beta Was this translation helpful? Give feedback.
-
|
this is for @fpliger ... we all seem to agree with the current MR and willing to re-iterate later on if the use case for disabling it per package makes sense (as opposite of globally, as that breaks other modules) ... is that OK to you? The MR is already approved: #1880 I just want to be sure we're all aligned in here, thanks! |
Beta Was this translation helpful? Give feedback.
-
|
That works for me too! Looking at the PR, it's only adding the patch and no disabling at all, right? Asking to explicitly confirm the intentions not for an issue per se. I think it's fine as is since js would mostly not work on a worker anyway (I think... It's early morning 😅) |
Beta Was this translation helpful? Give feedback.
-
|
correct, the PR just brings the document in so that matplotlib among other packages would just work from a worker, but of course if extra weird stuff is needed we can refine or rethink the patch |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Not only some Pyodide module assumes by default that code runs on the main thread, hence it reaches
js.documentwithout thinking that could be absent, we once again had some user finding the lack ofdocumentproblematic.I remember me and @fpliger also needed to manually patch that and point at the proxied version and I start wondering, as we already offer a
RUNNING_IN_WORKERconstant viapyscriptmodule, if we shouldn't just stretch further and enable more modules and reduce surprises when a scriptworkerattribute is used.That means that beside reaching out for
pyscript.windowandpyscript.document, the global Worker context gets polluted with both references pointing at our module.This makes the code easier to migrate but I am not sure this might create confusion, as w estill want people to understand that
pyscriptis the module they should use.However, we don't get to control 3rd party packages and if these assume, behind their scene, that
js.documentexists, we might avoid breaking out of the blue by monkey-patching that on our side after we are sure we're running in a worker.@ntoll @JeffersGlass @antocuni @madhur-tandon @leriomaggio ... thoughts?
Beta Was this translation helpful? Give feedback.
All reactions