improvement(client-ordered-collection): ConsensusQueueFactory generic#27497
improvement(client-ordered-collection): ConsensusQueueFactory generic#27497jason-ha wants to merge 3 commits into
Conversation
- Remove unused `IConsensusOrderedCollectionFactory` - Make `ConsensusQueueFactory` generic (assumes `unknown`) - Get specific on `create` & `load` return types - Remove most intended internal class use in test
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (115 lines, 5 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the ordered-collection DDS factory to be generic, removes the now-unneeded IConsensusOrderedCollectionFactory type, and adjusts tests/fuzz utilities to rely less on internal classes and more on public legacy typings.
Changes:
- Removed
IConsensusOrderedCollectionFactoryfrominterfaces.tsand public exports. - Made
ConsensusQueueFactorygeneric and tightenedcreate/loadreturn types. - Updated tests/fuzz utilities to use
ConsensusQueueFactory<T>and legacy imports/types.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dds/ordered-collection/src/test/fuzzUtils.ts | Updates fuzz model/state typings to use ConsensusQueueFactory<T>. |
| packages/dds/ordered-collection/src/test/consensusOrderedCollection.spec.ts | Switches test typings/imports to legacy public interfaces and adjusts class generics. |
| packages/dds/ordered-collection/src/interfaces.ts | Removes the IConsensusOrderedCollectionFactory interface definition. |
| packages/dds/ordered-collection/src/index.ts | Stops exporting IConsensusOrderedCollectionFactory. |
| packages/dds/ordered-collection/src/consensusOrderedCollectionFactory.ts | Introduces generic ConsensusQueueFactory<T> and updates ConsensusQueue shared-object-kind export. |
- symmetric explicit `<unknown>` - consistent `<string>` for fuzzUtils.ts - remove unused `<any>` on class value use
| * Test class that exposes protected applyStashedOp method for testing | ||
| */ | ||
| class TestConsensusQueue extends ConsensusQueueClass { | ||
| class TestConsensusQueue extends ConsensusQueueClass<unknown> { |
There was a problem hiding this comment.
Personally, I do not like the pattern of using generics such that they can cause incorrectly typed code to silently compile, especially when there is no runtime check involved.
There is no way for the TypeScript compiler to validate that the content stored in all possible current and future documents makes the type provided here, and not even a way for a careful developer to ensure its correct either. When importing persisted data, I don't think we should use generics to simply assume its what ever type the code feels like inferring, removing the compiler enforced validation of the data and hiding where the unsafe type conversion is occurring.
If you store Foos in your document, then a dev changes the Foo type in an incompatible way, or even uses a totally different type, there is nothing we can do at compile time to stop them, checked runtime conversion is required, and developers know where to do that because get type Unknown, and are forced to convert.
Anything that implicitly type converts content from persistence should require a runtime check: this is why we use strongly type schema like Tree schema and TypeBox. I'd rather not add violations to this typer safety practice in legacy code to make it slightly more ergonomic to write type unsafe usages of it.
There was a problem hiding this comment.
I don't think that is an opinion unique to you. I was equally put off when I saw what is going on here.
Unfortunately, we don't have all the time to go address past silliness. This at least moves the generic to unknown here instead of any.
IConsensusOrderedCollectionFactoryConsensusQueueFactorygeneric (assumesunknown)create&loadreturn types