alewin/useWorker

Proposals for 3.0.0

alewin opened this issue · 6 comments

alewin commented

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 a controller

Before:

const [sortWorker, workerStatus, killWorker] = useWorker(sortDates);
const res = await sortWorker()
function onButtonClick = () => workerController.killWorker()

After

 const [sortWorker, workerController] = useWorker(sortDates);
 const res = await sortWorker()
 function onButtonClick = () => workerController.killWorker()

PROs 👍

  • we can add any functionality through the controller without introducing breaking changes

CONS 👎:

  • Anywhing?

PROPOSAL 2- remove workerStatus

remove workerStatus from the values returned by thehook, and manage the status through a controller

Before:

  const [sortWorker, workerStatus, killWorker] = useWorker(sortDates);

  // workerStatus WORKER_STATUS.PENDING
  try{
    const res = await sortWorker()
    // workerStatus WORKER_STATUS.SUCCESS
  }catch(status, e){
     // workerStatus WORKER_STATUS.ERROR or WORKER_STATUS.TIMEOUT_EXPIRED
  }

After

 const [sortWorker, workerController] = useWorker(sortDates);

  let workerStatus = workerController.getStatus() // WORKER_STATUS.PENDING
  try{
    const res = await sortWorker()
    workerStatus = workerController.getStatus() // WORKER_STATUS.SUCCESS
  }catch(status, e){
    workerStatus = workerController.getStatus() // WORKER_STATUS.ERROR or WORKER_STATUS.TIMEOUT_EXPIRED
  }

PROs 👍

  • we can add any functionality through the controller without introducing breaking changes

CONS 👎:

  • the state of the worker will no longer be reactive through the react life cycle since it will no longer be a state but a simple variable)

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 worker

Before:

 const [sortWorker, sortWorkerStatus, killWorker] = useWorker(sortDates, {
    dependencies: [
      "https://cdnjs.cloudflare.com/ajax/libs/date-fns/1.30.1/date_fns.js"
    ],
  }
 const res = await sortWorker(dates)

After

const utils = {
    manipulate: { ....}
    add: {...}
}

const  sortDates = (date) => {
   var foo = utils.manipulate(date)
   return foo
}

 const [sortWorker, sortWorkerStatus, killWorker] = useWorker(sortDates, {
    remoteDependencies: [
      "https://cdnjs.cloudflare.com/ajax/libs/date-fns/1.30.1/date_fns.js"
    ],
   localDependencies: [{ utils: utils }]
  }
 const res = await sortWorker(dates)

Doubts 🤔:

  • it is probably useless because we could pass the utils object to the function

Example:

 const res = await sortWorker(dates, utils)

PROPOSAL 4 - useWorkers hook

Manage multiple workers with a new hook: useWorkers

Before:

const [sortWorker1, workerStatus1, killWorker1] = useWorker(sortDates);
const [numbersWorker, workerStatus2, killWorker2] = useWorker(sortNumbers);
const [sortWorke2r, workerStatus3, killWorker3] = useWorker(sortDates);

 const res1 = await sortWorker1(dates)
 const res2 = await numbersWorker(numbers)
 const res3 = await sortWorker2(dates)

function onButtonClick = () => killWorker2.killWorker()

After using Array

 const [workers] = useWorkers([sortDates, sortNumber, sortDates], options); // array or object?

 const res1 = await workers[0](dates)
 const res2 = await workers[1](numbers)
 const res3 = await workers[2](dates)

function onButtonClick = () => workers[1].killWorker()

After using Object

 const [workers] =  useWorkers({ sort1: sortDates, sort2: sortNumber, sort3: sortDates }, options);  // array or object?

 const res1 = await workers.sort1(dates)
 const res2 = await workers.sort2(numbers)
 const res3 = await workers.sort3(dates)

function onButtonClick = () => workers[1].killWorker()

Doubts 🤔:

  • should the first parameter be an array or an object?

PROs 👍

CONS 👎:

  • Anywhing?

PROPOSAL 5 - No-Hook

Allow the use and generation of a webworker without having to use a hook

PROs 👍

  • use of webworkers also within normal functions, or sagas

CONS 👎:

  • Anywhing?

Other proposals are welcome :)

xzilja commented

PROPOSAL 1 - remove killWorker

Makes total sense to me 👍

PROPOSAL 2- remove workerStatus

I think keeping it as a "reactive" status makes more sense, users might want to do something (update ui) when worker status changes?

PROPOSAL 3 - Inline dependencies

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.

PROPOSAL 4 - useWorkers hook

I think multiple useWorker hooks is more expressive / easier to manage? Having to do something like workers[1].killWorker() might get confusing, you'll have to know which index your specific worker is located in, so will have to scroll up to check that often. Might even make a mistake using incorrect index here.

PROPOSAL 5 - No-Hook

So how would this look? Similar to https://github.com/developit/greenlet ?

  • 👍3
  • 👀1
Pigotz commented

Proposals 1 and 2

I totally agree with @iljadaderko.

Proposal 4

This 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 scope

This 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 scope

This is really interesting for a really heavy task queue! It deserves to be explored more in depth and discussed with a dedicated RFC.

Proposal 5

I'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.

  • 👍3
zant commented

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 status will be the actual state. And we could also add killWorker from Proposal 1 or any other features there. This idea is inspired by the useQuery's hook API from Apollo, I really like it.

Proposal 3:
Agree again with Ilja, and I also think that as an opt-in feature it won't be pointless to add, if it's doable it'll be nice to see it implemented :)

Proposal 4:
Thread pool sounds really cool, and idem again, accessing by indexes could be an issue. However, we can find a way around that, in principle I like the idea and I'm really in to explore it.

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:
Agree with @Pigotz here, but we have unlimited repos right? ;)

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!

  • 👍2
  • 🎉1
z4o4z commented

hey, the proposals look really interesting, some notes I would like to mention:

PROPOSAL 2- remove workerStatus

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 status and add useStatus method(subscribes to the status updates under the hood) to the controller. Also, we can have getStatus method (non-reactive). I guess for some users status is not that important, so this approach can be useful for every user.

PROPOSAL 4 - useWorkers hook

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.

  • 👍2
Pigotz commented

So that we achieve the DX gains

@grdnrt I appreciate when someone cares about DX 💪

  • 😄1
alewin commented

PROPOSAL 1 - killWorker [Approved]

  • 👍 IljaDaderko, Pigotz, grdnrt, z4o4z

Solution: Reaplce kill worker with controller
PR: #39

PROPOSAL 2 - workerStatus [Rejected]

  • 👎IljaDaderko, Pigotz, grdnrt, z4o4z

Solution (grdnrt): mantein "reactive" status:
const [useWorker, { status, kill }] = useWorker(sortDates, options)
PR: #39

PROPOSAL 3 - Inline dependencies [Approved]

  • 👍 IljaDaderko, grdnrt

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]

  • 👍 Pigotz, grdnrt, z4o4z
  • 👎 IljaDaderko (accessing by indexes could be an issue)

Solution: the implementation will be discussed in a dedicated RFC, using object, or grdnrt proposal

Issue: #38

PROPOSAL 5 - no-hook [Rejected]

out of the original scope of this library, in the future we could think of extracting the core, in a separate repo

  • 🎉5