-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fixes #179 #180
Fixes #179 #180
Conversation
We were tracking flushing using one variable while there were in fact two types of flushing. This commit remedies that And furthermore adds a check for function streams whether their listeners need updating at the end of updateStream This also has some general code quality improvements including variable name changes
1bde30c
to
113eabf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks excellent to me, tried it out with no issues either. Great work!! I like the readability improvements as well!
I found one more problem with this. We would still see the same problems if we're doing nested streams in a stream body if we were flushing a stream value. This has been remedied now, and the fix broke the switchLatest module, so I have now updated the switchlatest module such that
It seems the switchLatest module only worked because of a bug 😂 I added some further readability improvements as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me
test/index.js
Outdated
@@ -1006,6 +1021,19 @@ describe('stream', function() { | |||
[], [1, 3, 2], [2, 8, 7, 6], [3, 5, 4] | |||
]); | |||
}); | |||
it('#179 nested streams atomic update', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what I feel about issue numbers in the test description. But, we can keep it if you find it useful 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference, Removed.
lib/index.js
Outdated
toUpdate.push(function() { | ||
updateStreamValue(s, n); | ||
}); | ||
updateLaterUsing(updateStreamValue_(n), s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using (s) => updateStreamValue(n, s)
instead? It seems a bit simpler and we avoid using the external curry function in the implementation itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I felt like I would have to use it in more places, but didn't notice it was only in a single place. So the curry has been removed.
The change made in the last commit accidentally removed the ability To update a stream often inside a stream body.
This helps with debugging (you don't have to step through ramda's curry while debugging)
Now that multi level dependent streams work we need switchLatest to not end its stream immediately because one of the streams in the merged end streams has a value right now.
…a stream value currently
2f1c552
to
1f831a7
Compare
LGTM! 🎉 This solves the problem nicely and in a clean manner. |
This issue was caused by #180 which made sure that nested streams were updated properly. Updating them properly caused nested takeUntil'd streams to end too soon.
This issue was caused by #180 which made sure that nested streams were updated properly. Updating them properly caused nested takeUntil'd streams to end too soon.
We were tracking flushing using one variable while there
were in fact two types of flushing
flushingStreamValue
, andflushingUpdateQueue
. This is now properly represented as two distinct case.At the end of the updateStream method I've added a check, that if we're not flushingStreamValue and the streams listeners should be updated then we call
s(s.val)
to force that update.Finally the update queue has been changed a bit. It now holds streams again. But the streams hold updater function arrays.
Hope you have time to review this soon @paldepind