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

Make sure useSelector.ts doesn’t create stale values #4168

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

enyo
Copy link
Contributor

@enyo enyo commented Jul 31, 2023

The function provided to svelte's readable is only invoked when the first subscriber subscribes to this store.

But because stately’s actor.subscribe does not fire immediately after subscribing (it only fires when something changes in the state) the svelte store could have a stale value (with the initial prevSelected) until the state in the machine changes and triggers the update.

This PR fixes that, by making sure that the value is not stale when the first subscription happens.

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2023

⚠️ No Changeset found

Latest commit: 904e6c4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 31, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 904e6c4:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented Jul 31, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@Andarist
Copy link
Member

Could you add a test case that would cover this case?

@enyo
Copy link
Contributor Author

enyo commented Aug 2, 2023

@Andarist done!

I think the comments in the code should be self descriptive, but here is a short summary since it’s pretty obscure Svelte behavior:

Using $count anywhere in the Svelte template, immediately creates a subscription of the store under the hood. That’s why it’s not possible to test this behavior with:

{#if $state.context.count >= 2}
  <div data-testid="selectorOutput”>{$count}</div>
{/if}

Instead I created another variable (readCount) that uses the get Svelte store function to read the value only when it’s supposed to be displayed (which means that the store is only subscribed to and read at a later time, and not from the start).

Without the fix in this PR, the value would still read 0 at the end, because the count store would still provide the initial value it received when it was created.

@enyo
Copy link
Contributor Author

enyo commented Aug 2, 2023

You might be wondering why this fix is needed in the first place, since you nearly always use a Svelte store in the template with the $count shorthand.

The bug appeared in our case, because we created the store and put it in a svelte context:

const count = useSelector(/* etc... */);
setContext(‘count’, count);

And then in some child component, at a later time, we used that store:

const count = getContext(‘count’);

And this is where the value was wrong.

@Andarist
Copy link
Member

Andarist commented Aug 2, 2023

I wonder, are you using @xstate/svelte@beta by copy-pasting it into your project? That's fine - I was just double-checking things and this package isn't yet released, hence the question.

@enyo
Copy link
Contributor Author

enyo commented Aug 2, 2023

I wonder, are you using @xstate/svelte@beta by copy-pasting it into your project? That's fine - I was just double-checking things and this package isn't yet released, hence the question.

Yes that’s exactly what I’m doing.

@Andarist Andarist merged commit 231a6f3 into statelyai:next Aug 2, 2023
2 checks passed
@enyo
Copy link
Contributor Author

enyo commented Aug 2, 2023

The same bug exists in v4, but since I’m using v5 and this is going to be the next version anyway, I thought I might just fix it in v5 (especially since that is an edge case that probably not many people encounter).

@enyo enyo deleted the patch-1 branch August 2, 2023 09:03
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