-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix: don't revert source to UNINITIALIZED state when time travelling #17409
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
Conversation
🦋 Changeset detectedLatest commit: 495f581 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| */ | ||
| capture(source, value) { | ||
| if (!this.previous.has(source)) { | ||
| if (value !== UNINITIALIZED && !this.previous.has(source)) { |
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 quite understand why we need this special case. If something is only getting initialized within a batch why not revert to uninitialized outside it? Wouldn't that mean the value is wrongfully bleeding into other batches while this one isn't committed yet?
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.
The point of this.previous is that if batch B is created while batch A is ongoing, we can commit batch B without the changes in batch A from being erroneously applied.
But in this scenario, the value hasn't changed — it was being initialized in batch A. It wouldn't make sense to revert it to an uninitialized state
elliott-with-the-longest-name-on-github
left a comment
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 fully understand this but the test looks legit, so...
This is an alternative to #17373, and fixes #17271. (The other PR does not fix the bug, unfortunately.)
I say 'partially' because it still fails when hydrating; in that case, effects that are created before anawaitis encountered run too soon. They should run once the component and all its children have finished doing whatever they need to do. As yet I haven't quite managed to figure out how to express that in code.This PR brings over the test from #17373 and adjusts it such that it fails on both
mainand that PR.Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint