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

Doing multiple changes inside transaction() results multiple invocations of observer subscribed via subscribe(). #184

Open
j-sen opened this issue Oct 19, 2021 · 3 comments

Comments

@j-sen
Copy link

j-sen commented Oct 19, 2021

Doing multiple changes inside transaction() results multiple invocations of observer subscribed via subscribe().

How to reproduce:

import 'https://unpkg.com/sinuous@0.27.12/dist/observable.js'
const {observable, subscribe, transaction, root}= self.observable
root(() => {
  const a = observable(1)
  const b = observable(2)
  const log = []
  subscribe(() => {
    log.push(a() + b())
  })
  transaction(() => {
    a(10)
    b(20)
  })
  // Expected  3;30
  // Got       3;12;30
  console.assert(log.join(';') === '3;30')
})

Link to https://CodePen.io/j-sen/pen/OJjNKVK

@nettybun
Copy link
Contributor

Unfortunately this is the programmed behaviour... I know it's not intuitive though. It comes down to this phrasing:

https://github.com/luwes/sinuous/blob/master/packages/sinuous/observable/src/observable.js#L51-L71

/**
 * Creates a transaction in which an observable can be set multiple times
 * but only trigger a computation once.
 * @param  {Function} fn
 * @return {*}
 */
...
  q.forEach(data => {
    if (data._pending !== EMPTY_ARR) {
      const pending = data._pending;
      data._pending = EMPTY_ARR;
      data(pending); // This calls all computeds linked to the observable
    }
  });

Each observable can only trigger a computation once, so a(1); a(2); a(3) will be ONE call to the subscribe. However, a(1); a(2); a(3); b(1); b(2) will be TWO calls because it's TWO observables and they each get one trigger.

I didn't like this behaviour so in my own reactive library (which is on indefinite hiatus/cancelled 😞) I implemented transaction() to call each observable/signal and gather all of the computeds/wires that would run into a Set() and then call them after only once.

https://github.com/heyheyhello/haptic/blob/main/src/state/index.ts#L288-L317

/**
 * Batch signal writes so only the last write per signal is applied. Values are
 * committed at the end of the function call. */
...
    signals.forEach((signal) => {
      // Doesn't run any subscribed wires since `transactionCommit` is set
      signal(signal.next); // Does not call computeds.
      delete signal.next;
      signal.wires.forEach((wire) => transactionWires.add(wire));
    });
    transactionCommit = false;
    _runWires(transactionWires) // Call computeds, once each, in the correct order.

@nettybun
Copy link
Contributor

You could make a PR for @luwes and he might accept it. Just collect computeds instead of running them and then batch call them afterwards. This would be a behavioural change tho...might break some apps.

@j-sen
Copy link
Author

j-sen commented Oct 29, 2021

@heyheyhello Thanks for your reply. It seems like a bad idea to make a PR if it's designed to work in this way. And I solved my problem by adding a flag. By the way, Haptic is cool.

@j-sen j-sen closed this as completed Oct 29, 2021
@luwes luwes reopened this Sep 18, 2022
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

No branches or pull requests

3 participants