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

[RDKit_minimal.js]: Assign initRDKitModule to WorkerGlobalScope if possible #326

Open
xuzuodong opened this issue Apr 23, 2023 · 7 comments

Comments

@xuzuodong
Copy link

xuzuodong commented Apr 23, 2023

Is your feature request related to a problem? Please describe.

We can only use RDKit.js inside a web worker in a legacy way:

// worker.js, a classic worker
importScripts("./RDKit_minimal.js")

initRDKitModule().then((rdkit) => {
    const mol = rdkit.get_mol(smiles)
    ...
    postMessage(...)
})
// use this classic worker file
const worker = new Worker("./worker.js")

While there is a preferred way to use web worker: use module type:

const worker = new Worker("./worker.js", { type: "module" })

In this way, we can't use importScripts anymore since now worker.js file is considered a module, and an error would occur:

image

So, I instead we should use import:

// worker.js, a module worker
import './RDKit_minimal.js'

// ...

Now here's the problem: I can't access the initRDKitModule function anymore since it didn't assigned to WorkerGlobalScope, so we can't directly do something like below, which I think would be an ideal way to use RDKit.js:

import './RDKit_minimal.js'

initRDKitModule() // uncaught ReferenceError: initRDKitModule is not defined

Describe the solution you'd like

initRDKitModule function should be assigned to WorkerGlobalScope in RDKit_minimal.js.

I tried and it seems to work by just adding these lines at the end of RDKit_minimal.js:

image

I think it should be added directly in the source code before JavaScript code generation, I don't know how to do it since it's written in c++.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

@MichelML
Copy link
Collaborator

MichelML commented Apr 23, 2023

Hi @xuzuodong, just by curiosity, have you tried writing:

import initRDKitModule from "./RDKit_minimal.js";

In other runtime like node.js, importing like above actually assigns initRDKitModule to globalThis, so I would guess it would be the same in a worker context. I must confess I have limited experience with web workers however, @atfera and @RamziWeslati seem to have use this in their rdkit-provider implementation , maybe they would have some thoughts on your issue here?

@xuzuodong
Copy link
Author

xuzuodong commented Apr 24, 2023

import initRDKitModule from "./RDKit_minimal.js";

@MichelML I tried this and won't work.

It turns out that if we want to use an outside function in an ES module web worker , we should either:

  1. expose the function using ES module export feature, i.e. export default initRDKitModule,
  2. or explicitly assign the function to WorkerGlobalScope, i.e. self.initRDKitModule = initRDKitModule

I think the second way would be better, because before implementing either way, we should first determine if user's runtime environment supports ESM, and it seems strenuous to do so in the first way.

@xuzuodong
Copy link
Author

xuzuodong commented Apr 24, 2023

I just created a demo, with Vite correctly handles the module worker.

image

A drawback is that RDKit_minimal.js and RDKit_minimal.wasm need to be placed in /src rather than /public, as Vite doesn't support importing executable JavaScript files from /public.

So this only works on development mode; on build time, .wasm file won't be packed into built bundle /dist/assets since RDKit_minimal.js doesn't work as a JavaScript module and import the wasm explicitly.

As a workaround, I used vite-plugin-static-copy to copy .wasm file to /dist/assets after build time. You can check related Vite config.

However, it's just not worthy to do so, since it brings more complexity to user's codebase than it trying to reduce.

It should be perfectly fixed if RDKit_minimal.js file can work as JavaScript module and Vite can handle import statement inside it.


So I guess now the true problem is that is it possible for RDKit.js distribute as an ES Module Build? If not, do you guys have plans to support it? If not, maybe this issue can be closed for now. 😃

@MichelML
Copy link
Collaborator

@xuzuodong sorry it took so long to answer here, @ptosco would you have any thoughts/comments on his last question?

@MichelML
Copy link
Collaborator

repoke @atfera @RamziWeslati here too

@MichelML
Copy link
Collaborator

@xuzuodong I will have a look at this myself real soon, sorry for the delays

@xuzuodong
Copy link
Author

Thank you so much for your continued attention to my issue! 🫶

I want to let you know that RDKit.js has been working perfectly for me with the RDKit.js related assets placed in the public directory of my Vite project. While addressing this specific issue would definitely be an icing on the cake, I understand that these things can take time. Please feel free to take the time you need!

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

No branches or pull requests

2 participants