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

feat: support choosing custom worker backend #290

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

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Aug 9, 2020

This adds a function to return a worker instance based on the provided options.

  • It allows us to choose between tiny and node threads.
    • using dynamic import only the needed function is imported
  • This is very handy for the environments that multiple workers are supported (like Electron).
  • The options later can be expanded if new features are added in the future.

Fixes #285
Fixes #229

@aminya aminya force-pushed the createWorker branch 2 times, most recently from c61dfa7 to d0d1615 Compare August 9, 2020 07:14
@andywer
Copy link
Owner

andywer commented Aug 16, 2020

Thanks for the PR, @aminya! Can you check why the rollup build is failing?

@aminya
Copy link
Contributor Author

aminya commented Aug 25, 2020

@andywer It works for me locally. It says getWorkerImplementation is not exported while it is! This is weird.

@aminya aminya force-pushed the createWorker branch 2 times, most recently from 2b10297 to 63f7119 Compare August 25, 2020 14:30
@aminya aminya force-pushed the createWorker branch 4 times, most recently from 5d4e4c6 to bce6d08 Compare January 10, 2021 03:28
@aminya
Copy link
Contributor Author

aminya commented Jan 10, 2021

@andywer This is ready to go.

@andywer
Copy link
Owner

andywer commented Jan 10, 2021

Thanks, @aminya. Need to sleep about it, though.

The issue is that this solution will only work in node.js, it won't work with webpack or parcel bundler.

@aminya
Copy link
Contributor Author

aminya commented Jan 10, 2021

Thanks, @aminya. Need to sleep about it, though.

The issue is that this solution will only work in node.js, it won't work with webpack or parcel bundler.

I have added the tests for Parcel bundler. Why do you say that it does not work?

@andywer
Copy link
Owner

andywer commented Jan 10, 2021

Need to double-check, but I think the only reason why the test doesn't fail is because the worker doesn't require any bundling when tested in a node.js environment.

Background: Parcel and the webpack plugin look for new Worker() expressions. Since the syntax for the selective implementation is createWorker(), neither will recognize the code as a worker instantiation anymore and won't create a new bundle for the referenced file.

@aminya
Copy link
Contributor Author

aminya commented Jan 10, 2021

I am OK with not exporting createWorker from the index.

BTW, tsconfig-esm is configured for es2015 which is not a good compiler source e.g.: it converts awaits to yield. We should consider using esnext for tesconfig-esm.

@andywer
Copy link
Owner

andywer commented Jan 13, 2021

Sorry, @aminya. Was super busy the last few days…

Yeah, maybe it's time to target a more up-to-date JS runtime like ES2017. This tsconfig.json has been around for two years – it were different times back then 😉

I am OK with not exporting createWorker from the index.

How would you use it then? Deep import á la import createWorker from "threads/createworker"?

@aminya
Copy link
Contributor Author

aminya commented Jan 13, 2021

Sorry, @aminya. Was super busy the last few days…

No worries!

Yeah, maybe it's time to target a more up-to-date JS runtime like ES2017. This tsconfig.json has been around for two years – it were different times back then 😉

Yes, I agree!

I am OK with not exporting createWorker from the index.

How would you use it then? Deep import á la import createWorker from "threads/createworker"?

Yes, like that. I am still not sure why this is required. Is it because of the dynamic imports?

@aminya
Copy link
Contributor Author

aminya commented Mar 27, 2021

@andywer I rebased this, and in the next commit, I removed createWorker export from index:

@andywer
Copy link
Owner

andywer commented Mar 27, 2021

Thanks so much, @aminya!

I will go through this PR once more in-depth before merging it, but it looks really good.

@aminya
Copy link
Contributor Author

aminya commented Apr 30, 2021

Any plan to merge this?

@andywer
Copy link
Owner

andywer commented Apr 30, 2021

@aminya Damn, I'm so sorry. Just swamped with work right now… 🙈

Feel free to annoy me on a regular basis as much as you like until this gets shipped. I will try to find some spare time.

@aminya
Copy link
Contributor Author

aminya commented Apr 30, 2021

I added some unit tests, but the code is super fragile. The resolved worker path depends on which file I import (../src/createWorker vs ../createWorker). Also, using an absolute path doesn't work!

@flux627
Copy link

flux627 commented Dec 31, 2021

Would be really nice to merge this

@DonovanDMC
Copy link

This is missing an export in the package, which makes it wholly unusable with esm from the get-go. Though. even after adding the export I couldn't get it to work.

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.

Ability to specify worker_threads/tinyworker Option for choosing the worker backend
4 participants