Arbor OST #146
Arbor OST #146
Conversation
Variables referencing OST nodes should be prefixed with '$' to indicate they are reactive, e.g. mutations triggered on them will trigger the subscription system to notify subscribers.
Detached properties do not exist in the OST, thus, when changes are made to them subscribers of the OST are not notified.
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new internal data structure called Observable State Tree (OST) to better streamline Arbor's internals. The key changes involve refactoring from a path-based system to a seed-based system and creating a new modular handler architecture.
- Replaces the existing
Seed.plant()method with a newplantSeedFor()approach - Implements a comprehensive OST architecture with handlers for different data types
- Adds extensive test coverage for the new OST implementation
Reviewed Changes
Copilot reviewed 106 out of 113 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/arbor-store/src/arbor.ts | Refactors seed management to use WeakMap-based tracking and new plantSeedFor method |
| packages/arbor-store/src/handlers/default.ts | Updates to use new tree-based seed planting approach |
| packages/arbor-store/tests/arbor.test.ts | Adds it.only to focus testing on state tree updates |
| packages/arbor-ost/ | New OST package with complete implementation including handlers, visitors, and comprehensive test suite |
| Various config files | Updates to project configuration including TypeScript version and tooling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| visit({ ost, target, $node }) { | ||
| return (...args: unknown[]) => { | ||
| let unshifted: unknown[] |
There was a problem hiding this comment.
The unshift method returns a number (the new length), but the variable is typed as unknown[]. The assignment should be unshifted = target.unshift(...args) and the return type should be number.
| let unshifted: unknown[] | |
| let unshifted: number |
| visit({ ost, target, $node }) { | ||
| return () => { | ||
| if (target.size === 0) { | ||
| return false |
There was a problem hiding this comment.
The clear method should return void like the native Map.clear(), not a boolean. Remove the return statements or return undefined.
| } | ||
| }) | ||
|
|
||
| return true |
There was a problem hiding this comment.
The clear method should return void like the native Map.clear(), not a boolean. Remove the return statements or return undefined.
| this.#seeds.set(value, new Seed()) | ||
| } | ||
|
|
||
| return this.#seeds.get(value) |
There was a problem hiding this comment.
The method creates a new seed but doesn't return it. The logic should return the newly created seed: if (!seed) { const newSeed = new Seed(); this.#seeds.set(value, newSeed); return newSeed; }
| this.#seeds.set(value, new Seed()) | |
| } | |
| return this.#seeds.get(value) | |
| const newSeed = new Seed(); | |
| this.#seeds.set(value, newSeed); | |
| return newSeed; | |
| } | |
| return seed; |
| thisArg?: unknown | ||
| ) { | ||
| for (const [key] of target.entries()) { | ||
| cb($node.get(key), key, thisArg || $node) |
There was a problem hiding this comment.
The third parameter should be the map itself, not thisArg. The call should be cb.call(thisArg, $node.get(key), key, $node).
| cb($node.get(key), key, thisArg || $node) | |
| cb.call(thisArg, $node.get(key), key, $node) |
Implement a new internal data structure called Observable State Tree to better streamline Arbor's internals.