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

Fix ending a stream while updating #229

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nordfjord
Copy link
Collaborator

Fixes #228 which highlighted an edge case that happens when a stream is ended while being processed in very specific circumstances. In those circumstances the end stream will be triggered before the listeners because it's updated is queued before the listener updates.

To solve for this we add a toEnd array into which the end stream of the current stream will be pushed to if it's updated while while the main stream is updated (that's a convoluted sentence, it's a convoluted data flow, not sure how to communicate this properly)

@nordfjord
Copy link
Collaborator Author

@paldepind I currently have permissions to merge to the repo, but I'm not able to publish flyd releases. Would appreciate, given you have some spare time, if you could review/merge/publish

@paldepind
Copy link
Owner

Thanks for fixing this @nordfjord. I'll look at the PR soon 🙂

Please share your npm username then I'll also add you to the npm package to give you publish rights 🙂

@nordfjord
Copy link
Collaborator Author

Thanks for fixing this @nordfjord. I'll look at the PR soon 🙂

Please share your npm username then I'll also add you to the npm package to give you publish rights 🙂

Awesome, thanks mate! My npm username is also nordfjord

@paldepind
Copy link
Owner

Thank you for looking into this @nordfjord! I haven't completely understood the issue yet. But, I wonder if the test could be simplified further? Do the details inside withLatestFrom matter for instance?

@nordfjord
Copy link
Collaborator Author

But, I wonder if the test could be simplified further?

Absolutely, I've attempted to whittle it down now! I'm honestly not sure how to communicate the issue, the dataflow to trigger it is quite strange

@nordfjord
Copy link
Collaborator Author

as an example of how edge casy this thing is, the issue doesn't manifest (on master) if I only combine a single stream.

Screenshot 2024-02-05 at 15 15 11 Screenshot 2024-02-05 at 15 15 31

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.

[Help Needed] CombineLatest in the style of RxJs
2 participants