Skip to content

fix: fixing asymmetric diamond dependency problem #31

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

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

divdavem
Copy link
Member

@divdavem divdavem commented Jun 15, 2022

This is a big refactoring of the algorithm so that the asymmetric diamond dependency problem is fixed:

const a = writable(0);
const b = derived(a, (a) => `b${a}`);
const c = derived([a, b], ([a, b]) => `${a}${b}`);
const values = [];
const unsubscribe = c.subscribe((value) => values.push(value));
a.set(1);
console.log(values); // displays ["0b0","1b0","1b1"], should display ["0b0","1b1"]

For info, the asymmetric diamond dependency bug is also present in svelte stores:
https://svelte.dev/repl/445983f3fdfe4db9ab860d88642f1989?version=3.48.0

A possible workaround (both in ngx-tansu and in svelte stores) is to transform it into the symmetric diamond dependency problem:

const a = writable(0);
const abis = derived(a, (a) => a); // add a useless level of derived store to work around the issue
const b = derived(a, (a) => `b${a}`);
const c = derived([abis, b], ([a, b]) => `${a}${b}`);
const values = [];
const unsubscribe = c.subscribe((value) => values.push(value));
a.set(1);
console.log(values); // correctly displays ["0b0","1b1"]

But it is better to have a proper fix. That's why I am opening this PR!

This PR makes invalidate calls recursive so that the whole tree of dependent stores is prevented from updating while values are changed. As a consequence, there is a need of the reverse revalidate operation (equivalent to next with the same value, but avoiding re-computations) in case the value does not change.
I have not measured the impact on performance in case the tree of dependencies is big. The invalidate listener functions should be kept as small and quick as possible (just switching a boolean and propagating the info).

This PR adds several tests for different cases in addition to the asymmetric diamond dependency case.

@divdavem divdavem force-pushed the asymmetricDependencyIssue branch from bc06eb3 to 6e0885f Compare June 16, 2022 12:47
@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #31 (9039b73) into master (ae35b18) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   98.46%   98.75%   +0.29%     
==========================================
  Files           1        1              
  Lines         130      161      +31     
  Branches       24       28       +4     
==========================================
+ Hits          128      159      +31     
  Partials        2        2              
Impacted Files Coverage Δ
src/index.ts 98.75% <100.00%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae35b18...9039b73. Read the comment docs.

@divdavem divdavem force-pushed the asymmetricDependencyIssue branch 22 times, most recently from e8555cb to d30077b Compare June 17, 2022 13:38
@divdavem divdavem marked this pull request as ready for review June 17, 2022 13:46
@divdavem divdavem requested review from maxokorokov and fbasso June 17, 2022 13:46
@divdavem
Copy link
Member Author

@pkozlowski-opensource Feel free to have a look at this PR and to give some feedback if you have time.

Copy link
Contributor

@fbasso fbasso left a comment

Choose a reason for hiding this comment

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

It is fine on my side, but probably need another look from another reviewer.

@maxokorokov, can you have a look ?

@pkozlowski-opensource
Copy link
Contributor

@divdavem oh, cool to see that you are digging into the batched and glitch-free propagation algorithm! I don't think I will have time to do any meaningful review so I don't want to block progress here.

@divdavem
Copy link
Member Author

@fbasso Thank you for your review!

@divdavem
Copy link
Member Author

@divdavem oh, cool to see that you are digging into the batched and glitch-free propagation algorithm! I don't think I will have time to do any meaningful review so I don't want to block progress here.

@pkozlowski-opensource Thank you for your comment! It is great to hear from you! I understand you are very busy!
By the way, did you receive my e-mail about npm rights to publish a new version of ngx-tansu?

@divdavem divdavem force-pushed the asymmetricDependencyIssue branch 2 times, most recently from 22c55eb to 49136b6 Compare June 17, 2022 14:47
@divdavem divdavem force-pushed the asymmetricDependencyIssue branch from 49136b6 to 9039b73 Compare June 17, 2022 16:01
Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, apart from invalidate/revalidate naming.

I find revalidate confusing, seems like uninvalidate would fit better, but I don't think it's a word. And I don't have better options.

freeze/unfreeze, pause/resume, disable/enable, I don't know...

@divdavem
Copy link
Member Author

@maxokorokov Thank you for your review!
I don't know either what names would be the best. I will integrate this PR as it is, then we can rename the functions in a separate PR.

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.

4 participants