Skip to content

Introduce level property to allow sorting of stores in computed#145

Merged
ai merged 9 commits into
nanostores:mainfrom
ogogorev:dependency-problem
Feb 2, 2023
Merged

Introduce level property to allow sorting of stores in computed#145
ai merged 9 commits into
nanostores:mainfrom
ogogorev:dependency-problem

Conversation

@ogogorev
Copy link
Copy Markdown
Contributor

@ogogorev ogogorev commented Jan 29, 2023

Let's consider following example:

let a = atom(0)
let b = computed(a, v => v)
let c = computed(b, v => v)
let d = computed([b, c], (b, c) => `${b} ${c}`)

d.subscribe((v) => {
  console.log('d', v);
});

The example above has following structure: A -> B, B -> C, BC -> D

It turns out that the problem is caused by the order in which listeners are added to the b store. The order of listeners follows the order of stores, passed to d. Since in the array [b, c] b comes first, d subscribes to b, then to c, and only then c subscribes to b. So b will notify d before c. Passing [c, b] solves the problem.

Letting users to manage the order of stores might not be the best option, since the relations between stores can be tricky and will cause bugs.

In my proposed solution I added to atom a new property level. Using level the computed store can sort passed stores in descending order and subscribe to them in the right order.

I also extended the test for the diamond problem, now it handles both A -> B, A -> C, BC -> D and A -> B, B -> C, BC -> D cases.

Intuition tells me that there might be more elegant solutions for the problem, especially using notifyId and listenerQueue. I hope at least my explanation of the issue will be useful.

Comment thread computed/index.test.ts Outdated
})

equal(values, ['a0b0'])
equal(values, ['000'])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you keep a0b0c0 thing here?

Copy link
Copy Markdown
Contributor Author

@ogogorev ogogorev Jan 30, 2023

Choose a reason for hiding this comment

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

Now I see why the letters are important: turned out there was a bug with the order of stores in callback. Thanks!

UPD: The letters are restored, bug is fixed.

@ai
Copy link
Copy Markdown
Member

ai commented Jan 30, 2023

I am right now in the travel and will think about it after tomorrow. Can you add more complex tests for now to be sure for edge cases?

@ogogorev
Copy link
Copy Markdown
Contributor Author

I am right now in the travel and will think about it after tomorrow. Can you add more complex tests for now to be sure for edge cases?

Sure, will do it tomorrow.

@ogogorev
Copy link
Copy Markdown
Contributor Author

ogogorev commented Jan 31, 2023

Well, I guess the order of stores passed to a computed was only a part of the problem.
I could come up with a working solution, but need more time to clean up and add even more tests before I share it. Will do it tomorrow.

@ogogorev
Copy link
Copy Markdown
Contributor Author

ogogorev commented Feb 1, 2023

@ai Take a look on it please.

Currently the level property is also added to currentListeners array, then in notify calls of listeners, whose levels are more than on step away from current level, are put in the end of listenersQueue.

Now it much more complicated than it was before, so I'm not sure it's worth it. But I also added some more complicated test cases, and almost all of them (2, 3, and 4) are failing, if I run them on the main branch.

Comment thread computed/index.test.ts
unsubscribe()
})

test('prevents diamond dependency problem 4 (complex)', () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I add a scheme for the data flow is this test? I think without a scheme nobody can understand it.

@ai
Copy link
Copy Markdown
Member

ai commented Feb 2, 2023

Thanks. Released in 0.7.2.

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.

3 participants