-
Notifications
You must be signed in to change notification settings - Fork 181
fix repeated renders when markReady called when collection already ready #604
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| "@tanstack/electric-db-collection": patch | ||
| "@tanstack/db": patch | ||
| --- | ||
|
|
||
| Fix repeated renders when markReady called when the collection was already ready. This would occur after each long poll on an Electric collection. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { | ||
| CollectionInErrorStateError, | ||
| CollectionStateError, | ||
| InvalidCollectionStatusTransitionError, | ||
| } from "../errors" | ||
| import { | ||
|
|
@@ -90,7 +91,18 @@ export class CollectionLifecycleManager< | |
| * Safely update the collection status with validation | ||
| * @private | ||
| */ | ||
| public setStatus(newStatus: CollectionStatus): void { | ||
| public setStatus( | ||
| newStatus: CollectionStatus, | ||
| allowReady: boolean = false | ||
| ): void { | ||
| if (newStatus === `ready` && !allowReady) { | ||
samwillis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // setStatus('ready') is an internal method that should not be called directly | ||
| // Instead, use markReady to transition to ready triggering the necessary events | ||
| // and side effects. | ||
| throw new CollectionStateError( | ||
| `You can't directly call "setStatus('ready'). You must use markReady instead.` | ||
| ) | ||
| } | ||
| this.validateStatusTransition(this.status, newStatus) | ||
| const previousStatus = this.status | ||
| this.status = newStatus | ||
|
|
@@ -129,9 +141,10 @@ export class CollectionLifecycleManager< | |
| * @private - Should only be called by sync implementations | ||
| */ | ||
| public markReady(): void { | ||
| this.validateStatusTransition(this.status, `ready`) | ||
| // Can transition to ready from loading or initialCommit states | ||
| if (this.status === `loading` || this.status === `initialCommit`) { | ||
| this.setStatus(`ready`) | ||
| this.setStatus(`ready`, true) | ||
|
|
||
| // Call any registered first ready callbacks (only on first time becoming ready) | ||
| if (!this.hasBeenReady) { | ||
|
|
@@ -146,12 +159,11 @@ export class CollectionLifecycleManager< | |
| this.onFirstReadyCallbacks = [] | ||
| callbacks.forEach((callback) => callback()) | ||
| } | ||
| } | ||
|
|
||
| // Always notify dependents when markReady is called, after status is set | ||
| // This ensures live queries get notified when their dependencies become ready | ||
| if (this.changes.changeSubscriptions.size > 0) { | ||
| this.changes.emitEmptyReadyEvent() | ||
| // Notify dependents when markReady is called, after status is set | ||
| // This ensures live queries get notified when their dependencies become ready | ||
| if (this.changes.changeSubscriptions.size > 0) { | ||
| this.changes.emitEmptyReadyEvent() | ||
| } | ||
|
Comment on lines
+162
to
+166
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this inside the |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -644,7 +644,7 @@ export class CollectionStateManager< | |
|
|
||
| // Ensure listeners are active before emitting this critical batch | ||
| if (this.lifecycle.status !== `ready`) { | ||
| this.lifecycle.setStatus(`ready`) | ||
| this.lifecycle.markReady() | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change fixed what the real cause of the stuck loading status bug from #532. We should always use
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we just remove setStatus? It's nice to have proper functions where we can put in more logic when needed.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed for this PR of course but a thought |
||
| } | ||
| } | ||
|
|
||
|
|
||
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.
We should prevent same status calls:
if (newStatus === this.status) returnThere 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 would like to address this in a follow-up pr, when this is done it triggers at least one test failure.