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

Local dependencies #98

Merged
merged 9 commits into from
Jan 23, 2021
Merged

Local dependencies #98

merged 9 commits into from
Jan 23, 2021

Conversation

zant
Copy link
Collaborator

@zant zant commented Jan 12, 2021

A new (and better) implementation of #77 thanks to @101arrowz and his cool lib isoworker.

Improvements over previous implementation:

  • Support for more primitives (classes, objects, symbols etc)
  • No need to use an object to access the functions, just using them normally (great one)

This is how it looks:

import { useWorker } from "@koale/useworker";
import { expensiveAdder } from './utils'

const fn = (a, b) => expensiveAdder(a,b) 

const [workerFn, {status: workerStatus, kill: workerTerminate }] = useWorker(fn, {
  timeout: 50000 // 5 seconds
  localDependencies: () => [expensiveAdder] // we pass the local function to the worker
});

I've updated the website and Readme with documentation regarding local deps, and also added some tests.

@zant zant requested a review from alewin January 12, 2021 15:15
This was referenced Jan 12, 2021
Copy link

@101arrowz 101arrowz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good, just a few minor tweaks would fix a few theoretical issues.

@@ -1,3 +1,4 @@
import isoworker from 'isoworker'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're using just createContext, maybe importing only that function is better. No need to bundle the workerization code.

Copy link
Collaborator Author

@zant zant Jan 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice suggestion, but I tried it and I'm getting the following:

Can't import the named export 'createContext' from non EcmaScript module (only default export is available)

Are you familiar with that message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @101arrowz, just saw your comment in the other discussion. This is the message I'm getting when changing it to a named export, do you have any ideas?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it should be exporting ES6 modules. There is a module field of package.json and there is an ESM build exported. Maybe try importing from fflate/browser or fflate/esm/browser.js.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't quite make it work with our current setup, could it be something related to our build config? @alewin

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zant seems strange but I'll try to check it tomorrow and update you here!

) => {
const [context] = isoworker.createContext(localDeps)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createContext returns a few other things, including some transferables that should be sent over. You should assign the received values of the transeferables to the global environment.

@alewin
Copy link
Owner

alewin commented Jan 12, 2021

Great work @zant 🥇 , and thanks @101arrowz for your great lib isoworker it's magic 👀

@alewin
Copy link
Owner

alewin commented Jan 16, 2021

I have published a beta containing the changes in this PR

npm i @koale/useworker@3.3.0-beta

https://www.npmjs.com/package/@koale/useworker/v/3.3.0-beta
@zant @101arrowz

@alewin
Copy link
Owner

alewin commented Jan 23, 2021

#77 (comment) 🎉 @zant @101arrowz

@zant zant force-pushed the feature/isoworker branch from eb5fb36 to 38930a8 Compare January 23, 2021 14:50
@alewin alewin merged commit 020a0ca into develop Jan 23, 2021
@alewin alewin linked an issue Jan 24, 2021 that may be closed by this pull request
@jer-sen jer-sen mentioned this pull request Mar 21, 2021
@alewin alewin deleted the feature/isoworker branch June 15, 2021 19:19
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.

RFC: Inline dependencies
3 participants