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 stateful subscribable overwriting updates #128

Merged
merged 1 commit into from Oct 15, 2021

Conversation

lemonmade
Copy link
Member

There was a particular edge case I was hitting with remote subscribables that I don't think is actually possible when using them across postMessage() (as they are intended to be used), but was possible when they ran in the same JavaScript environment (e.g., for tests):

  • The (sync) subscribable is created on the “host” side
  • We start the process of sending these subscriptions to the “remote” side and making them stateful, which includes subscribing to changes and sending the (potentially new) current value of the sync subscription (in case it has changed from the initial value)
  • After this has happened, but before the stateful subscription has received the "connection result" from the host, the host updates the value of the sync subscription. This invokes all the listeners, including the one registered by the remote side, and updates the stateful subscription on the remote side as we would expect
  • The "connection result" is received and has an outdated value, but we currently always use the value from that connection as being the most recent result

This PR fixes the issue by not taking the "connection result" value as the most up-to-date value if we have already started receiving updates through our subscriber.

@lemonmade lemonmade merged commit 14f7abf into main Oct 15, 2021
@lemonmade lemonmade deleted the fix-remote-subscribable-initial-value-overwrite branch October 15, 2021 13:45
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.

None yet

1 participant