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

RFC: Inline dependencies #77

Closed
zant opened this issue Aug 29, 2020 · 50 comments · Fixed by #98
Closed

RFC: Inline dependencies #77

zant opened this issue Aug 29, 2020 · 50 comments · Fixed by #98
Labels

Comments

@zant
Copy link
Collaborator

zant commented Aug 29, 2020

Hey everyone! I'm on my way to implement the Inline Dependencies feature from #37, which I decided to implement after a discussion on #69. (Details there).

I just finished the actual code and it's working just fine. However, there are small points on which I will be really glad to hear some opinions.

The first one is related to the way users will be accessing to the actual utils object, this is an example of how it works right now in my code:

const foo = (data) => {
  // Works just fine
  console.log(utils.fn())
  return data;
};

const utils = {
  fn: (val) => console.log(val)
};

const App = () => {
  const [worker, { terminateWorker }] = useWorker(foo, {
    transferable: "auto",
    localDependencies: utils, // Note how we the utils pass it directly
  });
}

This works because I'm inlining whatever object localDependencies is, and inserting it into the blob code, this is how it looks:

const utils = {fn: val => console.log(val),}

As you can see, it's named utils and that's the reason why it works inside foo. It's important to note that the utils object called inside foo is not the object with the same name declared below. This is important because of the following.

There can be the case in which the object passed to localDependencies isn't available in the scope of the foo function declaration, in which we will need to add a linter ignore rule, this of course, assuming that you're using a linter, which is fairly possible since CRA and most React libraries come with linters, as it almost required in modern development workflows.

const foo = (data) => {
  // eslint-disable-next-line no-undef
  console.log(utils.fooUtils.fn())
  return data;
};

const fooUtils = {
  fn: (val) => console.log(val)
};

const App = () => {
  const [worker, { terminateWorker }] = useWorker(foo, {
    transferable: "auto",
    localDependencies: { fooUtils },
  });
}

The above will still work, but as you can see, accessing correctly the properties inside the utils object, which lives in worker land:

// Trailing commas are in case there is more than one prop
const utils = {fooUtils: {fn: val => console.log(val),},}

That's really all of it, what do you think?

IMO, it's fairly understandable because the object is really not living on the function declaration's scope, so I think it gives clarity to what's going on under the hood, and can also be nicely integrated with a coincidentally good naming of your utils
object.

Another option could also be to pass the object as the last argument of foo, but I think that can be annoying since it will be really hidden under the hood.

Also, is utils a nice name for the inlined object in the worker? maybe localDependencies is a better suite?

P.D.: For reference, the implementation is at branch feature/27

@zant zant added the RFC label Aug 29, 2020
@zant
Copy link
Collaborator Author

zant commented Aug 29, 2020

cc @marco-m-alves

Let us know what you think 😄

@marco-m-alves
Copy link

Thanks for doing this so quickly.
How would this work in the use case where we have all of our util functions in specific js file/module and import those functions to make-up a complete processing function to be used within the useWorker(...) call?

import { fn1, fn2 } from './utils'

...

const App = () => {
  const [worker, { terminateWorker }] = useWorker(process, {
    transferable: "auto",
    localDependencies: { ???? },
  });
}

const process = (input) => {
  return ({
    a: fn1(input), b: fn2(input)
  })
}

@zant
Copy link
Collaborator Author

zant commented Aug 29, 2020

Hi @marco-m-alves! Good question.

That snippet will work just fine, you can inline the functions in the object passed to localDependencies

import { fn1, fn2 } from './utils'

...

const App = () => {
  const [worker, { terminateWorker }] = useWorker(process, {
    transferable: "auto",
    localDependencies: { fn1, fn2 }, // Inline here
  });
}

const process = (input) => {
  // Linter will complain about utils being not defined
  // eslint-disable-next-line no-undef
  return ({
    a: utils.fn1(input), b: utils.fn2(input) // Access utils object (which is the object inlined in the worker) and then the functions
  })
}

One thing that will definitely not work is to pass functions which depends on other functions or classes in the body. Because by the time the functions get evaluated, those will be missing on the worker side.
For example:

import * as Fuse from "fuse.js";
import { fn } from './utils'

// Passing this as an utils will not work, because depends on Fuse 
const fuseSearch = ({ list, options, input }, a) => {
  fn(); //  Will not work, fn will be not defined on worker side
  let fuse;
  if (list && options) fuse = new Fuse(list, options); // Will not work, Fuse will be not defined on worker side
  if (fuse && input) {
    return fuse.search(input);
  }
};

const App = () => {
  const [worker, { terminateWorker }] = useWorker(process, {
    transferable: "auto",
    localDependencies: { fuseSearch },
  });
}

You will need to explicitly give the worker all of the dependencies, fuse as a remote dep, and fn as a local dep.

@alewin
Copy link
Owner

alewin commented Aug 30, 2020

Sorry but for various personal reasons in the previous months, I was unable to take care of the library, issues and proposals..

Thanks for the support, and PR @gonzachr


That's really all of it, what do you think?

It might seem a little tricky, but probably also the cleanest way... 👍

Another solution could be: modify the function prototype, but it's alto very strange, and I don't like it very much. 👎

Example:

var foo = (nome) => `ciao ${nome}`
foo("alewin") // ciao alewin

Will become

var foo = (nome) => `${foo.localDeps.sayHello()} ${nome}`
foo.localDeps = { sayHello: () => 'ciao' }

foo("alewin") // ciao alewin

Also, is utils a nice name for the inlined object in the worker? maybe localDependencies is a better suite?

Yes I agree 👍

@zant
Copy link
Collaborator Author

zant commented Aug 30, 2020

Hi @alewin no worries, thanks for the feedback :)

That's actually an interesting suggestion, it is said that tweaking prototypes is not that good of a practice, but considering that it's somewhat tricky to get stuff into the worker, I think it can work quite well. I'll try to do some experiments with that this next week, try some edge cases and things. I also want to see if the prototype call can be hidden on the user end somehow.

I'll keep you guys updated 👍

@marco-m-alves
Copy link

Hi again. Any update on this? Thanks!

@zant
Copy link
Collaborator Author

zant commented Oct 21, 2020

Hi! @marco-m-alves sorry for not tuning in lately. I wasn't really able to get anything better than this proposal for this specific requirement.

The thing I tried to solve is the utils dep floating around which looks pretty weird, but haven't came up with anything better, as I think is a priority keep the library pretty minimal as it is, and posible solutions may introduce contexts or boilerplate which I think will not be the best for the library.

I think we can maybe release an alpha with this proposal and see how can we improve from there, I think it's pretty barebones so it shouldn’t gave us any trouble. What do you think? @alewin

@marco-m-alves
Copy link

Hi @gonzachr!

Thank you for the quick response.

I'm really looking forward to using this new feature.

@alewin
Copy link
Owner

alewin commented Oct 24, 2020

I agree! Unfortunately it remains a very difficult feature to implement :/
I will however release an alpha version with #78
@marco-m-alves @gonzachr

@zant
Copy link
Collaborator Author

zant commented Oct 24, 2020

Great news! Can you guide us in this process? I'm not too familiarized @alewin

@marco-m-alves
Copy link

Hi @alewin. Are we there yet? :)

Thanks again for the effort

@alewin
Copy link
Owner

alewin commented Nov 14, 2020

Great news! Can you guide us in this process? I'm not too familiarized @alewin

Sure! @zant
When publishing an npm package, you can associate a tag, by default the tag is latest so when you run the command npm publish the package will be available on @koale/useworker, in this case we want to create another tag, called beta.

The steps are:

  • npm login login to npm
  • npm install && npm run build create a build of useWorker.
  • edit package.json and package-lock.json version with the new version 4.0.0
  • npm publish --tag beta publishes all project files except those defined on .npmignore and .gitignore.

look in the "versions" tab of https://www.npmjs.com/package/@koale/useworker


Hi @alewin. Are we there yet? :)

I'm sorry to be late 😞
I just published a new package containing the @zant branch changes ( I haven't tested it yet )

npm i @koale/useworker@4.0.0

@zant
Copy link
Collaborator Author

zant commented Nov 14, 2020

Awesome, thanks for the explanation and new release @alewin!

We can now go ahead and start testing it @marco-m-alves. And please let us know how it goes, feedback will be highly appreciated :)

@marco-m-alves
Copy link

Thank you all for this effort. I’ll report back as soon as I’m able to test it out.

@101arrowz
Copy link

101arrowz commented Dec 8, 2020

@zant I've done something similar to this in my package fflate in these lines. That's a simplified version of my full findings; I actually found a way to support custom classes and instances of those classes, functions with any name, no need for a specific name for a wrapper object, etc. If you'd like, I can publish a package or write a summary of how it works.

@zant
Copy link
Collaborator Author

zant commented Dec 9, 2020

Hi @101arrowz thanks for reaching out! I think any of them will be super cool and useful (package or summary). We can go with whatever you think will be best 👌

@101arrowz
Copy link

Alright, I'll work on a package for this because it's quite complex functionality and could probably be reused by other projects. Effectively, what it does is regenerate the source code for whatever dependencies the user specifies with some clever techniques such as function.toString and walking prototype chains. The key part is that it works both in development (i.e. without transpilation) and production.

@101arrowz
Copy link

101arrowz commented Dec 10, 2020

@zant I've made a very basic prototype of what I was talking about at 101arrowz/isoworker. The createContext function accepts an array of dependencies with fixed names and returns a string that can be evaluated to recreate the dependencies in a worker context.

const isoworker = require('isoworker');
class Tmp {
  x = 2;
  func1() {
    ++this.x;
  }
  func2() {
    this.y = 10
  }
}
class Tmp2 extends Tmp {
  constructor() {
    super();
    this.x = 40;
  }
  func3() {
    this.x *= 2;
  }
  func2() {
    this.x /= 2;
  }
}
const var1 = new Tmp2();
const var2 = new Tmp();
const example = () => 'hi'
const context = isoworker.createContext(() => [var1, var2, example]);
// To make `self` exist (as in a worker scope)
let self = window; // `global` if in Node.js
eval(context); // Everything recreated

In practice, you'll embed the generated string into your worker with a Blob. I'll add functionality supporting auto-embedding in this package myself, but you can probably work off of what I wrote already.

@zant
Copy link
Collaborator Author

zant commented Dec 14, 2020

Hi @101arrowz, thanks for taking the time!

I've been playing with this on my machine and it's really cool. It definitely handles the encoding part way better than my naive implementation, and I also really liked the aspect which you mentioned as removing the need for specific names for wrappers.

This morning I tried to implement it in useWorker already to have a picture of how it will work but I'm having some issues regarding browser compatibility. For example, I had to change type: 'commonjs' on the tsconfig.json to esnext to be supported by the browser. But then I still had some troubles with the worker_threads dep, do you any suggestions?

Thanks again! :)

@101arrowz
Copy link

The package is compatible with IE10 and up. The issue with worker_threads is an easy fix: replace the first line of the file:

import wk from './node-worker';

with

import wk from './worker';

to resolve the issues. Most bundlers (Webpack, Parcel, Rollup) should support the way I configured it, which replaces the node-worker import with worker because the "browser" key of package.json specifies that replacement. However, some pure ESM builds do have trouble with it, which is why if you choose to actually implement the package, I'd create a browser-only build instead of a dynamic build. I did it the way I did for debugging mostly. Hope that clears it up!

@101arrowz
Copy link

@zant Did that end up working for you?

@zant
Copy link
Collaborator Author

zant commented Dec 22, 2020

Hi @101arrowz! Sorry couldn’t answer before. It didn’t work yet, but I think it also may be something with the way we're bundling, so I have to set time aside too look that up

@marco-m-alves
Copy link

Hi again.

I've tried 4.0.0 beta with a simple example and it seems to work as intended.

See code here:
https://codesandbox.io/s/vigilant-varahamihira-cboen?file=/src/App.js

Thanks!

@zant
Copy link
Collaborator Author

zant commented Jan 10, 2021

EDIT: We don't even need to use self, see @101arrowz response bellow.

Hi @marco-m-alves thank you for testing it! Looking good 👌

Also just to heads up, I've been working on the integration of the package made by @101arrowz which will allow us to write:

const thingy = () => console.log("hi");

const thingyFn = async () => {
  self.thingy();
};

function App() {
  const [thingyWorker] = useWorker(thingyFn, {
    localDependencies: () => [thingy],
  });
}

So we ditch using the confusing utils, in favor of self which is globally available both on web and worker land. And we will also be able to share objects, classes and other primitives around for free 👌

So I got it working super nicely @101arrowz, do you have any plans on publishing it to npm so we can add it to this library? 😄

Sorry for the delay though! Happy new year everyone :)

@101arrowz
Copy link

Yeah, I'll publish the package if you're on board with its design. I thought you had some issues with loading because of worker_threads, so I was holding off on publishing. I found a solution to the problem, so I'm going to publish with that fix.

Also of note is the fact that you don't explicitly need to use self.something() AFAIK. self is the global object in workers, so you can call it as if it were a normal function. In fact, in production, you can't use self.something() because terser doesn't rename something() to the minified name, but isoworker does.

const thingy = () => console.log("hi");

const thingyFn = async () => {
  thingy();
};

function App() {
  const [thingyWorker] = useWorker(thingyFn, {
    localDependencies: () => [thingy],
  });
}

I originally designed this so I could use synchronous code asynchronously without increasing bundle size, so if this doesn't work as if it were a synchronous function, I consider it a failure.

@zant
Copy link
Collaborator Author

zant commented Jan 10, 2021

Wow that's so cool! We really don't need to use self just tried it and works perfectly. Also thanks for the explanation 👍

Looking really good to me!

@101arrowz
Copy link

@zant, isoworker is on NPM. If something broke, just tell me and I'll see if I can fix it.

@zant zant mentioned this issue Jan 12, 2021
@zant
Copy link
Collaborator Author

zant commented Jan 12, 2021

Great news, with the amazing lib made by @101arrowz we have now full support for local dependencies on useWorker!

Implementation and details on #98

@marco-m-alves
Copy link

As I'm now incorporating the new approach on a more real world case, I encountered 2 "issues" that would be very nice if they could be solved:

(1) "utils" function referencing other function locally within the "utils" module
(2) which include the case in which "utils" function is recursive

If this currently possible, please let me know.

@101arrowz
Copy link

If you're asking if it's possible to make nested functions work without listing dependencies, unfortunately no. If you have a function in a utils file that calls a function local to that file, you won't be able to workerize it because it's not possible to get the source string of the called function.

One possible way to solve this would be allowing objects passed into the dependency list to have dep._dependencies = () => [func1, func2]. Instead of this:

const add = (a, b) => a + b;
const fib = i => i > 0 ? fib(i - 1) + fib(i - 2) : 1;
useWorker(fib, { localDependencies: () => [add] });

you could do:

const add = (a, b) => a + b;
const fib = i => i > 0 ? fib(i - 1) + fib(i - 2) : 1;
fib._deps = () => [add];
useWorker(fib);

This is more verbose, but it allows util functions to continue to hide local dependencies while providing them to useWorker. Of course, this isn't an optimal solution, but it is actually impossible to auto-detect dependencies. Not just infeasible, but impossible: the scope of those functions is often different from that in which the workerization function is called. Build tools exist for a reason, and this is one of them.

@marco-m-alves
Copy link

Thanks. My case is the following:

The utils module has 3 functions for which I have separate unit tests

// utils module
export const func1 = () => { ... };
export const func2 = () => { /* which is recursive / calls itself */ }:
export const func3 = () => { ... };

The worker function in the main code combines the 3 functions to produce the intended result:

// main code
import { func1, func2, func3 } from "./utils"
...
const workerFn = (params) => { /* calls func1, func2, func3 */ };
useWorker(workerFn, localDependencies: { func1, func2, func3 } );

It throws an error saying it can find func2 (the function that is recursive).
Also, I would like to have the workerFn defined also in the utils module if possible.
Note: i'm using useWorker 4.0.0

@alewin
Copy link
Owner

alewin commented Jan 16, 2021

I have published a new version of useWorker that use isoworker, based on #98

npm i @koale/useworker@3.3.0-beta

link https://www.npmjs.com/package/@koale/useworker/v/3.3.0-beta

@zant
Copy link
Collaborator Author

zant commented Jan 17, 2021

Hi @marco-m-alves, I think you'll get a much nicer implementation with the recently published version of useWorker noted above by @alewin 👍 It will be super cool if you try it and tell us how it went, thanks again!

@marco-m-alves
Copy link

marco-m-alves commented Jan 19, 2021

I'm getting a error on a simple example where I just import useWorker
Can't find variable: BigInt64Array

See link here: https://codesandbox.io/s/keen-hill-5vl98?file=/src/App.js

@alewin
Copy link
Owner

alewin commented Jan 19, 2021

@marco-m-alves there is nothing special inside you codesandbox... did you saved the file? 🤔

import React from "react";
import "./styles.css";
import { useworker } from "@koale/useworker";

export default function App() {
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <button>Go</button>
    </div>
  );
}

regarding Can't find variable: BigInt64Array I think depends on "Safari""

image

Safari doesn't support BigInt64Array and BigUint64Array that is user by isoWorker ( here:
https://github.com/101arrowz/isoworker/blob/d23dc052ca1bc1ce8b49a83477fa181ac56ee749/src/index.ts#L29 )..

@101arrowz do you have any solutions or ideas?

cc: @zant

@101arrowz
Copy link

101arrowz commented Jan 19, 2021

Yes, I can change that. I thought all browsers supported BigUint64Array, guess I'll have to change it.

EDIT: Just updated it now, try updating the dependency.

@zant
Copy link
Collaborator Author

zant commented Jan 19, 2021

Dependency updated on #98 👍

@alewin
Copy link
Owner

alewin commented Jan 19, 2021

thanks @marco-m-alves,

and thanks @zant @101arrowz super fast! 🚀

release beta2

npm i @koale/useworker@3.3.0-beta.2

@marco-m-alves
Copy link

I'm not being able to make 3.3.0-beta.2 work in a simple example — maybe i'm missing something obvious

undefined is not an object (evaluating '(0, n.default).createContext')

https://codesandbox.io/s/keen-hill-5vl98?file=/src/App.js

@101arrowz
Copy link

101arrowz commented Jan 20, 2021

I think this is because the export is .createContext, not .default.createContext. In other words, this (currently used) is not legal:

import isoworker from 'isoworker';
isoworker.createContext(...);

but this is:

import { createContext } from 'isoworker';
createContext(...);

@zant try updating the PR?

@zant
Copy link
Collaborator Author

zant commented Jan 20, 2021

Oh sure! Just @ you at a small question there @101arrowz :)

@marco-m-alves
Copy link

Hi again. Any news on the issue above?

If there is a new version available, I can test it over the weekend.

Thanks!

@alewin
Copy link
Owner

alewin commented Jan 22, 2021

Hi again. Any news on the issue above?

If there is a new version available, I can test it over the weekend.

Thanks!

Hi Marco, this weekend I'll try to fix the import issue, and i Will update you, thanks ;)

@marco-m-alves
Copy link

@alewin Great! thanks!

@alewin
Copy link
Owner

alewin commented Jan 23, 2021

Hi @marco-m-alves can you give me more information about your tests?

I created a repo using useWorker 3.3.0-beta.2 and seems to work
https://github.com/alewin/useworker-local-deps-demo

@marco-m-alves
Copy link

The link for the test is here — I run it in Safari for Mac
https://codesandbox.io/s/keen-hill-5vl98?file=/src/App.js

@alewin
Copy link
Owner

alewin commented Jan 23, 2021

@marco-m-alves
can you try this link? http://useworker-localdeps.surge.sh/

source: https://github.com/alewin/useworker-local-deps-demo/blob/main/src/App.js

image

@marco-m-alves
Copy link

It worked. Is it a problem with my code?

@alewin
Copy link
Owner

alewin commented Jan 23, 2021

It worked. Is it a problem with my code?

Oh no, your code was correct 🆗 , i think it is a problem with codesandbox, there are other open "issues" regarding codesandbox but which I have not yet investigated 😞

@alewin
Copy link
Owner

alewin commented Jan 24, 2021

Released https://github.com/alewin/useWorker/releases/tag/3.4.0

@alewin alewin closed this as completed Jan 24, 2021
@alewin alewin linked a pull request Jan 24, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants