-
Notifications
You must be signed in to change notification settings - Fork 106
Fix error propagation from sync clients to collections #671
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
base: main
Are you sure you want to change the base?
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -215,7 +215,7 @@ export class CollectionImpl< | |||||
| public utils: Record<string, Fn> = {} | ||||||
|
|
||||||
| // Managers | ||||||
| private _events: CollectionEventsManager | ||||||
| public events: CollectionEventsManager | ||||||
|
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. the manager classes are intended to be a private api, but have to be public so that they can see each other, and we can introspect them from tests. This should change it to public, but maintain the underscore on the name. The "public" face of the events api is a set of methods on the Collection class itself that forward to this manager.
Suggested change
|
||||||
| private _changes: CollectionChangesManager<TOutput, TKey, TSchema, TInput> | ||||||
| private _lifecycle: CollectionLifecycleManager<TOutput, TKey, TSchema, TInput> | ||||||
| private _sync: CollectionSyncManager<TOutput, TKey, TSchema, TInput> | ||||||
|
|
@@ -261,7 +261,7 @@ export class CollectionImpl< | |||||
| } | ||||||
|
|
||||||
| this._changes = new CollectionChangesManager() | ||||||
| this._events = new CollectionEventsManager() | ||||||
| this.events = new CollectionEventsManager() | ||||||
| this._indexes = new CollectionIndexesManager() | ||||||
| this._lifecycle = new CollectionLifecycleManager(config, this.id) | ||||||
| this._mutations = new CollectionMutationsManager(config, this.id) | ||||||
|
|
@@ -272,9 +272,9 @@ export class CollectionImpl< | |||||
| collection: this, // Required for passing to CollectionSubscription | ||||||
| lifecycle: this._lifecycle, | ||||||
| sync: this._sync, | ||||||
| events: this._events, | ||||||
| events: this.events, | ||||||
| }) | ||||||
| this._events.setDeps({ | ||||||
| this.events.setDeps({ | ||||||
| collection: this, // Required for adding to emitted events | ||||||
| }) | ||||||
| this._indexes.setDeps({ | ||||||
|
|
@@ -283,7 +283,7 @@ export class CollectionImpl< | |||||
| }) | ||||||
| this._lifecycle.setDeps({ | ||||||
| changes: this._changes, | ||||||
| events: this._events, | ||||||
| events: this.events, | ||||||
| indexes: this._indexes, | ||||||
| state: this._state, | ||||||
| sync: this._sync, | ||||||
|
|
@@ -810,7 +810,7 @@ export class CollectionImpl< | |||||
| event: T, | ||||||
| callback: CollectionEventHandler<T> | ||||||
| ) { | ||||||
| return this._events.on(event, callback) | ||||||
| return this.events.on(event, callback) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -820,7 +820,7 @@ export class CollectionImpl< | |||||
| event: T, | ||||||
| callback: CollectionEventHandler<T> | ||||||
| ) { | ||||||
| return this._events.once(event, callback) | ||||||
| return this.events.once(event, callback) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -830,7 +830,7 @@ export class CollectionImpl< | |||||
| event: T, | ||||||
| callback: CollectionEventHandler<T> | ||||||
| ) { | ||||||
| this._events.off(event, callback) | ||||||
| this.events.off(event, callback) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -840,7 +840,7 @@ export class CollectionImpl< | |||||
| event: T, | ||||||
| timeout?: number | ||||||
| ) { | ||||||
| return this._events.waitFor(event, timeout) | ||||||
| return this.events.waitFor(event, timeout) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,7 +78,7 @@ export class CollectionLifecycleManager< | |
| loading: [`initialCommit`, `ready`, `error`, `cleaned-up`], | ||
| initialCommit: [`ready`, `error`, `cleaned-up`], | ||
| ready: [`cleaned-up`, `error`], | ||
| error: [`cleaned-up`, `idle`], | ||
| error: [`cleaned-up`, `idle`, `loading`], | ||
| "cleaned-up": [`loading`, `error`], | ||
| } | ||
|
|
||
|
|
@@ -144,6 +144,11 @@ export class CollectionLifecycleManager< | |
| * @private - Should only be called by sync implementations | ||
| */ | ||
| public markReady(): void { | ||
| // If recovering from error, transition to loading first | ||
| if (this.status === `error`) { | ||
| this.setStatus(`loading`) | ||
| } | ||
|
Comment on lines
+147
to
+150
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. I don't like this, We do not have a way in live queries to handle a How much handling of errors and moving between states to we expect the sync implementation to do? It seems to me there are two options for a collection:
Option 2 has some nuances arround subscriptions we would need to handle. Existing subscription would also need to be restarted, but we had not implemented that yet, it would be the truncate message from issue #634 proposing a refactor of the reconciliation process. |
||
|
|
||
| this.validateStatusTransition(this.status, `ready`) | ||
| // Can transition to ready from loading or initialCommit states | ||
| if (this.status === `loading` || this.status === `initialCommit`) { | ||
|
|
@@ -170,6 +175,15 @@ export class CollectionLifecycleManager< | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Mark the collection as being in an error state | ||
| * This is called by sync implementations when persistent errors occur | ||
| * @private - Should only be called by sync implementations | ||
| */ | ||
| public markError(): void { | ||
| this.setStatus(`error`) | ||
| } | ||
|
|
||
| /** | ||
| * Start the garbage collection timer | ||
| * Called when the collection becomes inactive (no subscribers) | ||
|
|
||
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.
Whats this?