-
Notifications
You must be signed in to change notification settings - Fork 101
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
Proposals for 3.0.0 #36
Comments
Makes total sense to me 👍
I think keeping it as a "reactive" status makes more sense, users might want to do something (update ui) when worker status changes?
I'd prefer local dependencies, seems cleaner and more expressive, also if function is reused in multiple places, you won't need to pass dependencies to it multiple times, just once when defining the hook.
I think multiple
So how would this look? Similar to https://github.com/developit/greenlet ? |
Proposals 1 and 2I totally agree with @iljadaderko. Proposal 4This is cool! But I think we are mixing a lot of stuff together here: do you want a higher level API around multiple workers with different scopes or something to manage a Thread Pool? Multiple workers, different scopeThis might be a simple code snippet inside the documentation: you could just prepare something ready to be copy-pasted for the develop who has this kind of problem Multiple workers, same scopeThis is really interesting for a really heavy task queue! It deserves to be explored more in depth and discussed with a dedicated RFC. Proposal 5I'd appreciate a separation between something more core, like the raw creation of the WebWorker, and something more high level, like the Hook itself. Sadly, this goes a little out of the original scope of this library. |
Hey, some nice proposals here! And great comments so far. On Proposal 2: As @iljadaderko, I'll go for maintaining the reactiveness of the status. To tackle this issue, I'll suggest to expose a plain object instead of an instance. So that we achieve the DX gains while maintaining reactiveness. Maybe something like this: const [useWorker, { status, killWorker }] = useWorker(sortDates, options) Where Proposal 3: Proposal 4: Besides that, something we can do to [possibly] maintain and control the workers could be to use a closure to maintain a list with the state of each worker there. I imagine it kind of like how React implements hooks. Something like: const workerCreator = () => {
const workers = []
// shenanigans
return (func) => {
const [caller, controller] = createWorker(func) // this will behave how useWorker does now
workers.push(controller)
return [caller, controller]
}
}
// here we capture the pool in the closure and return the same useWorker API
const useWorker = workerCreator()
// and then use that to create the workers normally but in the background will be tracked
const [sumWorker, controller] = useWorker(x => x + 1) Of course the above could be totally mistaken but wanted to share the idea! 😅 Proposal 5: In general really cool stuff. I'm excited to see where the library goes and all the cool stuff that can come up in the process. Thanks for sharing the ideas @alewin great work! |
hey, the proposals look really interesting, some notes I would like to mention:
personally I agree with @iljadaderko that "reactive" status makes more sense, so my idea here is to leave the status as is(the second item of the array) and add worker controller as the third item. OR we can remove
I really like this idea, but as for me, passing objects are more useful since it is easier to read/control the code, you do not care about the order. With the arrays, if the order of workers is changed developer should change the order of all other places, with objects there's no such problem. and it is easier to pass dependencies for each worker by worker name, not be index. |
@grdnrt I appreciate when someone cares about DX 💪 |
PROPOSAL 1 - killWorker [Approved]
Solution: Reaplce kill worker with PROPOSAL 2 - workerStatus [Rejected]
Solution (grdnrt): mantein "reactive" status: PROPOSAL 3 - Inline dependencies [Approved]
Solution: the implementation will be discussed in a separate task. const [sortWorker, sortWorkerStatus, killWorker] = useWorker(sortDates, {
remoteDependencies: [
"https://cdnjs.cloudflare.com/ajax/libs/date-fns/1.30.1/date_fns.js"
],
localDependencies: [{ utils: utils }]
} Issue: #37 PROPOSAL 4 - useWorkers [Approved]
Solution: the implementation will be discussed in a dedicated RFC, using object, or grdnrt proposal Issue: #38 PROPOSAL 5 - no-hook [Rejected]
|
These are 3/4 braking changes that I would like to introduce in version:
3.0.0
I chose to transcribe them so if someone wants to participate or give an opinion we can discuss it together 😃@iljadaderko @Pigotz @gonzachr @JonatanSalas @z4o4z
PROPOSAL 1 - remove killWorker
remove
killWorker
from the values returned by thehook
, and manage the status through acontroller
Before:
After
PROs 👍
CONS 👎:
PROPOSAL 2- remove workerStatus
remove
workerStatus
from the values returned by thehook
, and manage the status through acontroller
Before:
After
PROs 👍
CONS 👎:
PROPOSAL 3 - Inline dependencies
currently the worker dependencies are urls that will be injected through
importScripts
, since react-scripts still does not support web workers, we could allow to inject functions and objects into the workerBefore:
After
Doubts 🤔:
Example:
PROPOSAL 4 - useWorkers hook
Manage multiple workers with a new hook:
useWorkers
Before:
After using Array
After using Object
Doubts 🤔:
array
or anobject
?PROs 👍
CONS 👎:
PROPOSAL 5 - No-Hook
Allow the use and generation of a webworker without having to use a hook
PROs 👍
CONS 👎:
Other proposals are welcome :)
The text was updated successfully, but these errors were encountered: