saasquatch / bunshi

Molecule pattern for jotai, valtio, zustand, nanostores, xstate, react and vue
https://www.bunshi.org/
MIT License
218 stars 16 forks source link

Reduce calls to `molecule()` callback #39

Closed davidisaaclee closed 8 months ago

davidisaaclee commented 10 months ago

I've just started trying this library today – it's very nice – thank you!

I noticed that molecule() constructors are called very often – see https://codesandbox.io/s/awesome-butterfly-3n3gnl?file=/molecules.ts

In that demo (forked from a CodeSandbox from another issue in this repo), CountMolecule essentially calls console.log(Math.random()) in its constructor. Running the demo, you can see this being printed to console (with a new value for Math.random() as expected) many times for each button click. These constructor calls do not affect the value of the molecule (which makes sense).

If a molecule constructor runs expensive code, these "extra" calls would be an issue. Even with cheap constructors, my current use of Bunshi—which has many molecule calls per scope instead of a single one returning a set—racks up hundreds of calls to constructors per frame.

This seems unnecessary to me – are there any workarounds that reduce these calls?

I haven't dived too deep, but it seems like these "extra" constructor calls are called to detect dependencies (i.e. calls to mol / scope in the constructor). If this reading is correct, I think my ideal API here would be to split the dependencies and value constructor into two fields:

const counterMolecule = molecule({
  value: () => 0,
  deps: (_mol, scope) => scope(CounterScope)
});

// A downside of this approach is that you have to be sure to mark
// dependencies when you use an upstream value (very similar problem
// to react-hooks/exhaustive-deps)
const counterText = molecule({
  value: (mol) => `Counter: ${mol(counterMolecule)}`,
  deps: (mol) => mol(counterMolecule)
});

In this form, there could be even more performance gain by specifying deps as an immutable array and not a callback (so you wouldn't need to get the list of dependencies on every reference), but this would lose the ability to have dynamic deps like if (needsScope) { scope(myScope); } (although I don't see a use case for that).

An alternative that is more easily backwards compatible (I think this works, I'm not too familiar with generators):

// molecule constructor can be optionally specified as a generator.
// When Bunshi sees a generator, Bunshi calls the constructor generator
// up to the `yield` to get dependencies. If Bunshi needs to continue to get
// the initial value, it resumes execution of the generator; otherwise, it bails
// and avoids incurring the cost of the rest of the constructor.
const counterText = molecule(function* (mol, _scope) {
  const counterValue = mol(counterMolecule);
  yield;
  return `Counter: ${counterValue}`;
});
loganvolkers commented 10 months ago

This is an interesting problem and one that is being explored in work to resolve #35 as well.

You can see the unit tests on the related branch that confirm that the useMolecule call is being called multiple times.

There are a couple workarounds that are possibly by changing the internal state of injector: 1) When the molecule callback is called, it's dependencies are cached forever so they can't be changed. This would prevent conditional dependencies. 2) When the molecule callback is called, it's dependencies are cached, and the molecule is never called again unless scope values change. This would allow conditional dependencies.

Here is an example of a conditional dependency:

const scopeDependent = molecule((mol,scope)=>{

   if(scope(IsEnabled)){
     scope(ComponentScope);
  }

});

I think the ideal solution to this problem:

  1. Reduces unnecessary calls to the callback
  2. Allows conditional dependencies
  3. Doesn't require a change to the molecule syntax or API
  4. Is well tested through unit tests
davidisaaclee commented 10 months ago

I think the second "generator" approach I suggested could hit all of those points in terms of API. (Not sure if it matches Doesn't require a change to the molecule syntax or API, although I don't see how it would be possible to maintain current API and add this behavior – with the current dependency setup, you can't know dependencies until you run the constructor – you can assert that the deps won't change but that seems like it'd be confusing, since the constructor can say one thing but the internal state of bunshi will say another... just reread your (2) approach, clever)

I'm probably not looking to work on this until it becomes an issue in my own project, but:

  1. Would you be open to a solution (like one of the ones I suggest here) in parallel to #35?
  2. What is a use case of a conditional dependency? I'm not convinced that it's a useful feature (but again, I haven't been using Bunshi for very long).
loganvolkers commented 10 months ago

I have a solution for this issue in the branch for #35

It's basically the "Goldilocks" or "just right" cache implementation.

It needs a lot more unit tests to make sure it's complete. Solving this issue was actually much easier than solving #35.

loganvolkers commented 8 months ago

@davidisaaclee this has been fixed in 2.1.0-rc.2 can you please give it a try in your application? I'd like to get it validated before releasing an official version.

It does not support conditional dependencies, but should cover your use case.

davidisaaclee commented 8 months ago

@loganvolkers nice, thank you for addressing this. Unfortunately, I ended up replacing Bunshi, so I can't check this in my project. Feel free to mark this as closed if you think it is completed!