refactor the Collection class into multiple manager classes#560
refactor the Collection class into multiple manager classes#560
Conversation
🦋 Changeset detectedLatest commit: 77e8303 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: +3.88 kB (+5.61%) 🔍 Total Size: 73 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 1.44 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the large Collection class into multiple focused manager classes to improve maintainability and reduce complexity. The monolithic Collection class was broken down into specialized managers for different responsibilities while maintaining the same public API.
Key Changes:
- Split Collection class into multiple manager classes (state, changes, lifecycle, sync, indexes, mutations)
- Moved internal state properties from public access to
_statenamespace - Updated all test files to use new import paths and access patterns
Reviewed Changes
Copilot reviewed 78 out of 79 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/collection/index.ts | New main collection implementation using manager pattern |
| packages/db/src/collection/state.ts | State management and optimistic updates logic |
| packages/db/src/collection/changes.ts | Event emission and subscription handling |
| packages/db/src/collection/lifecycle.ts | Status management and garbage collection |
| packages/db/src/collection/sync.ts | Synchronization logic and preloading |
| packages/db/src/collection/indexes.ts | Index creation and management |
| packages/db/src/collection/mutations.ts | Insert, update, delete operations |
| All test files | Updated import paths and state access patterns |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Copilots review complained of the circular dependancy between |
|
@kevin-dp either as part of your PR, a followup, or this one if it goes in second, we should consider moving |
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
Splitting the Collection class into helper classes was absolutely necessary, great job with that, it is way more understandable and maintainable now!
The main Collection class (in index.ts) still has quite a bit of boilerplate code because we need to define public methods that just forward the call to the right nested instance and every of these new classes needs to keep a reference back to the collection...
I really wish Typescript had support for traits or multiple inheritance. It would be nicer and more concise if we could simply extend each behavior and we would inherit these methods instead of having to define them and forward the calls. That way the classes also wouldn't need to keep a reference back to the collection.
There is a way to achieve mixins in TS, here's a snippet from chatgpt showing how it can be done:
type Ctor<T = {}> = new (...args: any[]) => T;
function Auth<TBase extends Ctor>(Base: TBase) {
return class extends Base {
login(u: string, p: string) {/*...*/}
logout() {/*...*/}
};
}
function Sync<TBase extends Ctor>(Base: TBase) {
return class extends Base {
syncNow() {/*...*/}
scheduleSync(ms: number) {/*...*/}
};
}
class Core { /* shared state */ }
class Big extends Sync(Auth(Core)) {
ping() { return "pong"; }
}
const big = new Big();
big.login("k","pw");
big.syncNow();I do think it's worth rewriting this to use this mixins pattern. Wdyt?
|
@kevin-dp thanks! Yep that mix-in pattern is nice. The one drawback is that the inner mix-in doesn't see the types of the props on the outer mix-in (I don't think?). We would need to central abstract interface still. We have a quite a lot of two way dependancies through the separate functionality. very open to suggestions on this |
|
@samwillis You're right that inner ones don't know about state and methods introduced by outer ones. In typical OOP with traits (or abstract classes with multiple inheritance) each trait can define abstract state/methods it requires. We can also leverage abstract properties/methods in the mixin pattern. So, we can have the base class define abstract state/methods and each mixin can be defined as an abstract class (such that TS doesn't complain if they don't implement (all) the abstract properties/methods): type Ctor<T = {}> = abstract new (...args: any[]) => T;
abstract class Core {
abstract hookSchedule(ms: number, f: () => void): void;
}
function Scheduler<TBase extends Ctor<Core>>(Base: TBase) {
abstract class Scheduler extends Base {
hookSchedule(ms: number, f: () => void) {
setTimeout(f, ms);
}
};
return Scheduler
}
function NeedsScheduler<TBase extends Ctor<Core>>(Base: TBase) {
abstract class NeedsScheduler extends Base {
doThing() {
this.hookSchedule(10, () => {/* ... */});
}
};
return NeedsScheduler
}
class Big extends NeedsScheduler(Scheduler(Core)) {} |
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
LGTM! Thanks for taking the time to improve this
| TSchema extends StandardSchemaV1 = StandardSchemaV1, | ||
| TInput extends object = TOutput, | ||
| > { | ||
| private collection!: CollectionImpl<TOutput, TKey, any, TSchema, TInput> |
There was a problem hiding this comment.
What is the ! at the end of the name? Is this a TS thing or some convention i don't know?
There was a problem hiding this comment.
These don't have an initialiser in the constructor - they are set in setDeps, so we either have to mark them as optional (then check in every method), or do this which says "I know I don't have an initialiser but I promise it will be there before I access it".
We can revisit this pattern later if you want and add a check in each method, but I think for this use case in an internal api it's fine.
Collectionhad grown too large and complex - this moves everything into manager classes.It also moves a bunch of the internal, albeit publicly accessible, state into
_stateand other underscore prefixed class instances. Of note is thesyncedDatamap, this was public at the top level so that it was accessible in tests and to the Transaction system. It is now under_state.syncedDatamaking it a little more private. If anyone depended on this its moved, but I think thats good as its an internal detail that they should not be depending on.