Skip to content

Local dependencies#98

Merged
alewin merged 9 commits into
developfrom
feature/isoworker
Jan 23, 2021
Merged

Local dependencies#98
alewin merged 9 commits into
developfrom
feature/isoworker

Conversation

@zant

@zant zant commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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
Closed
Closed
@zant zant mentioned this pull request Jan 12, 2021
Open
101arrowz
101arrowz reviewed Jan 12, 2021
View reviewed changes

@101arrowz 101arrowz left a comment

Copy link
Copy Markdown

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.

Comment thread src/lib/createWorkerBlobUrl.ts
@@ -1,3 +1,4 @@
import isoworker from 'isoworker'

@101arrowz 101arrowz Jan 12, 2021

Copy link
Copy Markdown

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.

@zant zant Jan 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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?

@zant zant Jan 20, 2021

Copy link
Copy Markdown
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?

@101arrowz 101arrowz Jan 20, 2021

Copy link
Copy Markdown

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.

@zant zant Jan 21, 2021

Copy link
Copy Markdown
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

@alewin alewin Jan 21, 2021

Copy link
Copy Markdown
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!

Comment thread src/lib/createWorkerBlobUrl.ts
transferable: TRANSFERABLE_TYPE,
localDeps: () => unknown[],
) => {
const [context] = isoworker.createContext(localDeps)

@101arrowz 101arrowz Jan 12, 2021

Copy link
Copy Markdown

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

alewin commented Jan 12, 2021

Copy link
Copy Markdown
Owner

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

@alewin

alewin commented Jan 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Owner

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
alewin requested changes Jan 19, 2021
View reviewed changes
Comment thread package.json Outdated
@alewin

alewin commented Jan 23, 2021

Copy link
Copy Markdown
Owner

#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
RFC: Inline dependencies #77
Closed
@jer-sen jer-sen mentioned this pull request Mar 21, 2021
Open
@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

Reviewers

@alewin alewin alewin requested changes

+1 more reviewer

@101arrowz 101arrowz 101arrowz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RFC: Inline dependencies

3 participants

@zant @alewin @101arrowz