[DTP-963] Add support for customer provided typings for LiveObjects #1922
Conversation
WalkthroughThe pull request introduces significant enhancements to the TypeScript definitions for the Ably Realtime and REST client library. Key changes include the addition of generic type parameters in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
test/package/browser/template/src/ably.config.d.ts (1)
3-4: Well-structured global type declaration.The global interface declaration follows TypeScript best practices for type augmentation. This approach allows users to extend the types globally while maintaining type safety.
Consider adding JSDoc comments to document:
- The purpose of this interface
- How users should extend or customize it
- Any constraints or requirements for custom types
test/package/browser/template/src/index-liveobjects.ts (4)
5-6: Remove outdated TODO commentThis TODO comment can be removed as the file already implements comprehensive checks for types exported by the LiveObjects plugin.
-// TODO: check we can refer to types exported by LiveObjects plugin. -
20-34: Consider expanding type validation test coverageThe current tests cover primitive types and nested maps well, but could be enhanced with:
- Array type validations
- Complex object type validations
- Edge cases like null vs undefined
Consider adding these additional test cases:
// Array type validation const anArray: string[] = root.get('arrayKey'); // Complex object validation interface ComplexType { nested: { field: number; }; } const complex: ComplexType = root.get('complexKey');Also, the comment about nullish coalescing could be more explicit:
-// need to use nullish coalescing as we didn't actually create any data on the root, -// so the next calls would fail. we only need to check that TypeScript types work +// Note: These assertions use nullish coalescing (?.) because this is a type-only test. +// In a real scenario, the root would contain actual data. We're only validating +// that TypeScript correctly enforces the type constraints.
Line range hint
35-47: Enhance subscription callback test coverageWhile the exhaustive type checking is good, consider:
- Testing all possible update types explicitly
- Verifying unsubscribe operation success
// Verify all update types and unsubscribe result const { unsubscribe } = root.subscribe(({ update }) => { switch (update.someKey) { case 'removed': const removedValue: string = update.oldValue; break; case 'updated': const newValue: string = update.newValue; const oldValue: string = update.oldValue; break; default: const exhaustiveCheck: never = update.someKey; } }); // Verify unsubscribe returns void or a promise const unsubResult: void | Promise<void> = unsubscribe();
Line range hint
48-55: Expand LiveCounter type validation testsThe current counter tests could be more comprehensive to ensure type safety across all counter operations.
Consider adding these test cases:
// Verify counter operation types type IncrementResult = ReturnType<typeof counter.increment>; // should be Promise<number> type DecrementResult = ReturnType<typeof counter.decrement>; // should be Promise<number> // Verify subscription update types counter?.subscribe(({ update }) => { switch (update.type) { case 'increment': const incValue: number = update.inc; break; case 'decrement': const decValue: number = update.dec; break; default: const exhaustiveCheck: never = update.type; } }); // Verify value bounds if applicable type CounterValue = ReturnType<typeof counter.value>; // should be numbersrc/plugins/liveobjects/liveobjects.ts (1)
45-49: Documentation looks good, consider adding an example.The documentation clearly explains the default behavior and use case for explicit typing. Consider adding a code example to demonstrate both default and explicit type usage.
Example to add:
// Default usage const root = await liveObjects.getRoot(); // With explicit type interface CustomType { count: number; items: string[]; } const typedRoot = await liveObjects.getRoot<CustomType>();src/plugins/liveobjects/liveobjectspool.ts (1)
Line range hint
1-211: Consider documenting type customization examples.The implementation robustly handles LiveObjects with proper synchronization and error handling. Given that this change is part of adding support for customer-provided typings, it would be valuable to add documentation or code examples showing how users can customize
LiveMapTypefor their use cases.Consider adding:
- JSDoc comments explaining the type customization capabilities
- Code examples in the README or documentation showing how to extend
LiveMapType- Type constraints or validation for custom types if applicable
ably.d.ts (1)
2048-2048: Consider Cross-Platform Compatibility forStateValueTypesThe
StateValuetype includesBuffer, which is Node.js-specific and may not be available in all environments. Consider usingArrayBufferorUint8Arrayfor better cross-platform compatibility.Suggested Change: Use
ArrayBufferInstead ofBuffer-export type StateValue = string | number | boolean | Buffer | Uint8Array; +export type StateValue = string | number | boolean | ArrayBuffer | Uint8Array;This ensures that the code remains compatible across different JavaScript environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/liveobjectspool.ts(2 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)
🔇 Additional comments (19)
test/package/browser/template/src/tsconfig.json (2)
4-4: LGTM! Enabling strict null checks improves type safety.
This is a positive change that aligns well with the PR's goal of supporting customer-provided typings. Strict null checks will help catch potential null/undefined issues at compile time, making the template project more robust.
4-4: Verify the impact of strict null checks on the template project.
Since this change is in the template directory, we should verify that:
- All template code properly handles null/undefined values
- The template remains a good starting point for users
✅ Verification successful
Let me try a different approach to verify the impact of strict null checks.
Enabling strictNullChecks is appropriate and properly handled in the template
The template code demonstrates proper handling of null/undefined values through TypeScript's type system and safe runtime checks:
- Type definitions in
ably.config.d.tscorrectly mark optional properties with? index-liveobjects.tsshows appropriate null handling patterns:- Optional chaining (
?.) for safe property access - Non-null assertions (
!) are used intentionally where type narrowing is clear from context - Variables explicitly typed with union types including
undefined
- Optional chaining (
- Runtime checks are present (e.g.,
if (!response.ok)insandbox.ts)
The template serves as a good example of TypeScript null safety practices, making it a suitable starting point for users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential null/undefined issues in template TypeScript files
# Find all TypeScript files in the template directory and check for null-related patterns
echo "Checking for potential null/undefined handling in template files:"
fd -e ts -e tsx . test/package/browser/template/src \
--exec rg -l "null|undefined|\\?" {} \
--exec echo "Found null/undefined usage in {}"
# Check if there are any type assertions that might be bypassing null checks
echo -e "\nChecking for type assertions that might bypass null checks:"
ast-grep --pattern '(_ as $_)' test/package/browser/template/src
Length of output: 2935
Script:
#!/bin/bash
# First, let's find all TypeScript files in the template directory
echo "TypeScript files in template:"
fd -e ts -e tsx . test/package/browser/template/src
# Check for null/undefined handling patterns
echo -e "\nChecking for null/undefined handling:"
rg "null|undefined|\?" test/package/browser/template/src
# Look for optional chaining or nullish coalescing
echo -e "\nChecking for optional chaining/nullish coalescing:"
rg "\?\.|??" test/package/browser/template/src
# Check for non-null assertions
echo -e "\nChecking for non-null assertions:"
rg "!" test/package/browser/template/src
# Look for any compiler errors or warnings in build output
echo -e "\nChecking tsconfig content:"
cat test/package/browser/template/src/tsconfig.json
Length of output: 3013
test/package/browser/template/src/ably.config.d.ts (3)
1-2: LGTM! Clean and focused imports.
The imports are specific and minimal, importing only the required types from the 'ably' package.
1-19: Verify consistency with other type definitions.
The implementation successfully enables customer-provided typings as intended. However, we should ensure consistency with any similar type definitions across the codebase.
#!/bin/bash
# Search for similar interface definitions
ast-grep --pattern 'interface $interface {
root: {
$$$
};
}'
# Search for LiveMap usage patterns
rg -t typescript "LiveMap<" --glob '!test/**'5-17: Consider type structure improvements for better flexibility and safety.
While the type structure is well-defined, there are several potential improvements to consider:
- The literal types ('bar', 'qux') might be too restrictive for real-world use cases
- Deep nesting of LiveMap types could impact performance
- Consider adding readonly modifiers for immutable properties
Consider this alternative structure:
root: {
numberKey: number;
stringKey: string;
booleanKey: boolean;
couldBeUndefined?: string;
mapKey?: LiveMap<{
- foo: 'bar';
+ foo: string;
nestedMap?: LiveMap<{
- baz: 'qux';
+ baz: string;
}>;
}>;
counterKey?: LiveCounter;
};Let's verify the usage of these types in tests:
test/package/browser/template/src/index-liveobjects.ts (1)
Line range hint 1-55: Verify complete coverage of customer-provided typings
Let's ensure all aspects of customer-provided typings are tested.
✅ Verification successful
Let me gather more information about the type definitions to ensure complete coverage.
Based on the gathered information, I can now provide the final response:
Customer-provided typings are adequately tested ✓
The test file comprehensively covers all aspects of the customer-provided typings functionality:
- Tests primitive types (number, string, boolean)
- Validates optional/undefined types
- Verifies nested LiveMap structures with string literal types
- Confirms LiveCounter type integration
- Tests exhaustive type checking in update callbacks
The implementation aligns perfectly with the type definitions in ably.config.d.ts and demonstrates proper type inference and validation for all declared types in the LiveObjectsTypes interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the completeness of typing tests
# Check for the presence of ably.config.d.ts which should define LiveObjectsTypes
echo "Checking for type definitions file..."
fd -t f "ably.config.d.ts"
# Look for any other test files that might contain LiveObjects typing tests
echo "Checking for additional LiveObjects tests..."
rg -t ts "LiveObjectsTypes" --files-with-matches
# Verify all exported types from the LiveObjects plugin are tested
echo "Checking for untested LiveObjects types..."
ast-grep --pattern 'export (type|interface) $name' src/plugins/liveobjects/
Length of output: 599
Script:
#!/bin/bash
# Check the content of ably.config.d.ts to understand defined types
echo "Content of ably.config.d.ts:"
cat test/package/browser/template/src/ably.config.d.ts
# Check if all LiveObjects types are being tested
echo -e "\nChecking LiveObjects type usage in test file:"
rg -A 5 "LiveObjectsTypes" test/package/browser/template/src/index-liveobjects.ts
# Look for any exported types in the plugin
echo -e "\nLooking for exported types in plugin:"
rg -t ts "export (type|interface)" .
Length of output: 16578
src/plugins/liveobjects/liveobjects.ts (1)
50-50: Verify type compatibility with existing code.
The generic type implementation looks good and aligns with the PR objectives. The type assertion is correctly applied to preserve type safety.
Let's verify the type compatibility:
Also applies to: 56-56
✅ Verification successful
Type implementation is compatible with existing codebase
The verification confirms that:
- The generic type implementation in
getRoot<T extends API.LiveMapType = API.DefaultRoot>()matches the type definition inably.d.ts - The type constraint and default type are correctly aligned with the
LiveMap<T>class implementation - Existing usage in tests shows no type-related issues
- The implementation maintains backward compatibility while adding type safety through generics
The type changes are safe and properly integrated with the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing LiveMap usage patterns and potential breaking changes
# Check for existing getRoot() calls that might be affected
rg -A 2 "getRoot\(\)"
# Check for LiveMap type usage patterns
ast-grep --pattern 'LiveMap<$_>'
# Look for existing type definitions that might need updates
fd "\.d\.ts$" --exec rg -l "LiveMap"
Length of output: 8389
Script:
#!/bin/bash
# Let's check the LiveMap type definition and its usage with generics
# Check LiveMap type definition
rg -A 10 "LiveMap\s*(<|{|=)" ably.d.ts
# Check for any existing generic type usage with LiveMap
rg -A 2 "LiveMap<" --type ts
# Check the specific file for context around the getRoot implementation
cat src/plugins/liveobjects/liveobjects.ts
Length of output: 11319
src/plugins/liveobjects/liveobjectspool.ts (2)
3-3: LGTM! Clean type-only import.
The import statement correctly uses the type keyword for importing type definitions, following TypeScript best practices.
197-197: Verify LiveMap generic type implementation.
The type enhancement looks good, but we should verify the compatibility between LiveMap<API.LiveMapType> and its usage in the method.
Let's verify the type definitions and implementation:
✅ Verification successful
Based on the shell script results, I can now generate the final response as the implementation has been verified:
LiveMap generic type implementation is correctly defined and used
The type enhancement is properly implemented:
LiveMap<T>class correctly extendsLiveObjectwith the generic constraintT extends API.LiveMapTypeAPI.LiveMapTypeis well-defined as a recursive type that can contain nested maps and counters- The
zeroValuestatic method and constructor are properly typed with the generic parameter - Usage in
liveobjectspool.tsaligns with the type definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check LiveMap generic type implementation and API.LiveMapType definition
# Check LiveMap class definition
ast-grep --pattern 'class LiveMap<$_> {
$$$
}'
# Check LiveMapType definition in ably.d.ts
rg -A 5 'LiveMapType' --type ts
Length of output: 3562
src/plugins/liveobjects/livemap.ts (3)
4-4: Importing API types improves clarity
The import of * as API enhances type references and improves code readability.
50-50: Addition of generic type parameter enhances type safety
Introducing the generic type parameter T extends API.LiveMapType allows for stronger type enforcement and flexibility in the LiveMap class.
130-132: Ensure type safety when casting returned values
Casting data.value and the result of get to T[TKey] may not be type-safe if the actual types do not align. Consider verifying that these values conform to T[TKey] or adjust the types accordingly.
Run the following script to check the types of data.value and get(data.objectId):
✅ Verification successful
Let me gather more context about the types and usage by examining the class and method definitions.
Let me check one more thing to understand the StateData type and its relationship with T[TKey].
Based on the gathered information, I can now provide the final response:
Type casting is safe in this context
The type casting is safe because:
LiveMap<T>enforcesT extends LiveMapTypewhereLiveMapTypeonly allowsStateValue | LiveMap<LiveMapType> | LiveCounter | undefined- The
data.valuecast is used only when'value' in datais true, ensuringdataisValueStateData - The
get(data.objectId)cast is used for the complementary case, ensuringdataisObjectIdStateData
Both cases are properly guarded by type checks and align with the T[TKey] constraint from the LiveMapType definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where `data.value` and `get(data.objectId)` are used and their types.
# Find all instances of `data.value` and `get(data.objectId)`
rg -A 3 "return data\.value as T\[TKey\]"
rg -A 3 "return this\._liveObjects\.getPool\(\)\.get\(data\.objectId\) as T\[TKey\]"
Length of output: 620
Script:
#!/bin/bash
# Get the class and method definition containing these casts
ast-grep --pattern 'class $_ {
$$$
get($_): $_ {
$$$
return data.value as T[TKey];
$$$
}
$$$
}'
# Find the type definition of data
rg -B 10 "data\.value as T\[TKey\]"
# Check for any type definitions or interfaces related to this
rg "interface.*Map" -A 5
rg "type.*Map" -A 5
Length of output: 8596
Script:
#!/bin/bash
# Find StateData type definition and related types
rg "type.*StateData|interface.*StateData" -A 10
rg "class LiveMap.*extends" -B 5 -A 10
Length of output: 3583
ably.d.ts (7)
2032-2032: Enhancement: Introduce Generic Type Parameter in getRoot Method
The getRoot method in the LiveObjects interface now includes a generic type parameter T with a default of DefaultRoot. This enhances flexibility and type safety when retrieving the root object.
2045-2046: Approval: Correct Definition of LiveMapType
The LiveMapType definition correctly allows for nested maps, counters, and state values, enabling flexible and recursive data structures.
2047-2055: Approval: Appropriate Use of Conditional Types in DefaultRoot
The DefaultRoot type uses conditional types to determine the appropriate root type based on user definitions. This provides a sensible default and ensures type safety.
2056-2057: Approval: Generic Implementation of LiveMap Interface
The LiveMap<T> interface correctly introduces a generic type parameter and defines the get method to ensure type safety when accessing keys.
Line range hint 2059-2063: Approval: Definition of LiveCounter Interface
The LiveCounter interface is appropriately defined, extending LiveObject with the correct update type and providing a value method.
Line range hint 2065-2070: Approval: Generic LiveObject Interface Implementation
The LiveObject<TUpdate> interface and its methods subscribe, unsubscribe, and unsubscribeAll are correctly defined, ensuring extensibility and type safety for various live object updates.
2037-2042: 🛠️ Refactor suggestion
Avoid Polluting the Global Namespace with LiveObjectsTypes Interface
Declaring LiveObjectsTypes in the global scope can lead to conflicts and unintended side effects due to global namespace pollution. Consider using module augmentation to allow users to extend the interface without affecting the global scope.
Suggested Change: Use Module Augmentation
Modify the code to avoid declaring in the global scope:
- declare global {
- export interface LiveObjectsTypes {
- [key: string]: unknown;
- }
- }
+ export interface LiveObjectsTypes {
+ [key: string]: unknown;
+ }Users can then extend LiveObjectsTypes in their own code using module augmentation:
// In the user's codebase
declare module 'ably' {
interface LiveObjectsTypes {
root: MyCustomRootType;
}
}⛔ Skipped due to learnings
Learnt from: VeskeR
PR: ably/ably-js#1880
File: liveobjects.d.ts:26-26
Timestamp: 2024-11-12T07:31:53.691Z
Learning: For plugins like `LiveObjects`, the `any` type is intentionally used to maintain flexibility.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
0e63675 to
b7eeff6
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
90f38a7 to
5d79580
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/plugins/liveobjects/livemap.ts (1)
114-133: Consider improving type safety with proper type narrowingThe current implementation relies heavily on type assertions. We can improve type safety by leveraging TypeScript's type narrowing capabilities.
Consider this refactored implementation:
- get<TKey extends keyof T & string>(key: TKey): T[TKey] { + get<TKey extends keyof T & string>(key: TKey): T[TKey] | undefined { const element = this._dataRef.data.get(key); if (element === undefined || element.tombstone === true) { - return undefined as T[TKey]; + return undefined; } // data exists for non-tombstoned elements const data = element.data!; if ('value' in data) { - return data.value as T[TKey]; + return data.value as T[TKey]; // Single cast needed due to runtime type erasure } else { - return this._liveObjects.getPool().get(data.objectId) as T[TKey]; + const obj = this._liveObjects.getPool().get(data.objectId); + return obj as T[TKey]; // Single cast needed due to runtime type erasure } }Changes:
- Made
undefinedan explicit part of the return type- Combined the undefined checks
- Removed unnecessary type assertions for
undefined- Added explanatory comments for remaining necessary type assertions
ably.d.ts (1)
2056-2058: Consider adding JSDoc comments for LiveMap interfaceWhile the interface definition is correct, adding JSDoc comments would improve developer experience by providing inline documentation for the methods.
Add documentation like this:
export declare interface LiveMap<T extends LiveMapType> extends LiveObject<LiveMapUpdate> { + /** + * Gets the value associated with the specified key. + * @param key - The key of the value to get + * @returns The value associated with the key + */ get<TKey extends keyof T & string>(key: TKey): T[TKey]; + /** + * Gets the number of key/value pairs in the map. + * @returns The size of the map + */ size(): number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/liveobjectspool.ts(2 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plugins/liveobjects/liveobjects.ts
- src/plugins/liveobjects/liveobjectspool.ts
- test/package/browser/template/src/ably.config.d.ts
- test/package/browser/template/src/index-liveobjects.ts
- test/package/browser/template/src/tsconfig.json
🔇 Additional comments (7)
src/plugins/liveobjects/livemap.ts (3)
4-4: LGTM: Type-only import correctly added
The type-only import for API types is appropriately added to support the generic type constraints.
50-50: LGTM: Generic type parameter correctly constrained
The generic type parameter T extends API.LiveMapType properly enforces type safety while allowing customer-defined types, aligning with the PR objectives.
66-70: Skip: Issue already identified
The missing generic type parameter in the constructor call was already identified in a previous review comment.
ably.d.ts (4)
2032-2032: LGTM: Type-safe generic parameter addition to LiveObjects interface
The addition of the generic type parameter T extends LiveMapType = DefaultRoot to getRoot() improves type safety by allowing users to specify their own types while maintaining backward compatibility through the default type.
2037-2042: LGTM: Well-structured global interface for user-defined types
The LiveObjectsTypes global interface provides a clean and flexible way for users to define their own types. The string index signature with unknown value type offers maximum flexibility while maintaining type safety.
2044-2045: LGTM: Clear and flexible LiveMapType definition
The LiveMapType definition effectively captures the structure of LiveMap objects with appropriate type constraints. The union type for values covers all possible cases while preventing invalid types.
2047-2054: LGTM: Sophisticated type inference with helpful error messages
The DefaultRoot type implementation is well thought out:
- Uses conditional types to check for user-provided types
- Falls back to default
LiveMapTypewhen no types are provided - Includes a descriptive error message when invalid types are provided
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
b7eeff6 to
f03ca11
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
5d79580 to
d93e9f5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ably.d.ts (1)
2044-2054: LGTM: Well-designed type definitions with excellent type safetyThe
LiveMapTypeandDefaultRoottypes provide robust type checking with helpful error messages. The conditional type implementation elegantly handles both user-provided and default cases.Consider improving the error message readability:
- : 'Provided type definition for the "root" object in LiveObjectsTypes is not of an expected LiveMapType type'; + : 'Invalid type for "root" in LiveObjectsTypes: Expected a type that extends LiveMapType';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(4 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)src/plugins/liveobjects/liveobjectspool.ts(2 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/plugins/liveobjects/livemap.ts
- src/plugins/liveobjects/liveobjects.ts
- src/plugins/liveobjects/liveobjectspool.ts
- test/package/browser/template/src/ably.config.d.ts
- test/package/browser/template/src/index-liveobjects.ts
- test/package/browser/template/src/tsconfig.json
🔇 Additional comments (3)
ably.d.ts (3)
2032-2033: LGTM: Well-designed generic type support for LiveObjects
The updated getRoot method signature with generic type parameter T extends LiveMapType = DefaultRoot provides excellent type safety while maintaining backward compatibility. This change effectively enables custom typing support as intended.
2037-2042: LGTM: Well-implemented global interface for type customization
The global LiveObjectsTypes interface provides an elegant solution for user-defined type augmentation. The index signature with unknown type offers maximum flexibility while maintaining type safety.
2056-2058: LGTM: Excellent type-safe implementation of LiveMap
The LiveMap interface with generic type parameter and type-safe get method provides strong typing guarantees. The use of mapped types ensures that accessed properties return their correct corresponding types.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
f03ca11 to
1fe3fd3
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
d93e9f5 to
b140811
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(5 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/liveobjects.ts
- test/package/browser/template/src/ably.config.d.ts
- test/package/browser/template/src/index-liveobjects.ts
- test/package/browser/template/src/tsconfig.json
🔇 Additional comments (7)
src/plugins/liveobjects/livemap.ts (4)
3-3: LGTM!
The import of API types is necessary for the new generic type parameter and follows proper module organization.
49-49: LGTM! Well-designed type parameter constraint.
The generic type parameter with its constraint to API.LiveMapType provides type safety while enabling customer-provided typings, which directly addresses the PR objective.
Line range hint 63-76: LGTM! Proper type propagation in factory methods.
The static factory methods correctly propagate the generic type parameter, ensuring type safety is maintained throughout object creation and state restoration.
86-105: Consider adding runtime type checking for returned values.
While the type system ensures compile-time safety, there's no runtime validation that the stored values match the expected types in T[TKey]. Consider adding runtime type checking, especially for the value property.
Let's check if there are any existing type validation utilities in the codebase:
Consider implementing a type validation utility that can be shared across the codebase for consistent runtime type checking.
ably.d.ts (3)
2032-2032: Method signature change enhances type safety and flexibility
The update to getRoot with a generic type parameter allows users to define custom typings for the root object, improving type safety and adaptability.
2037-2043: Introduction of global 'LiveObjectsTypes' interface enables user-defined typings
Declaring the global LiveObjectsTypes interface allows users to provide their own types for LiveObjects, enhancing the developer experience.
2056-2057: Generics in 'LiveMap' interface improve type inference
Updating LiveMap to use generic type parameters enhances strict typing of keys and values, improving type safety and developer experience.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
1fe3fd3 to
9da78a6
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
b140811 to
f513bc6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/plugins/liveobjects/livemap.ts (2)
3-3: Consider using path aliases for better maintainability.The deep relative import path (
../../../ably) could be simplified using TypeScript path aliases. This would make the imports more maintainable and less prone to errors when files are moved.Consider configuring path aliases in your
tsconfig.json:{ "compilerOptions": { "paths": { "@ably/*": ["ably/*"] } } }Then update the import:
-import type * as API from '../../../ably'; +import type * as API from '@ably/types';
Line range hint
49-105: Consider documenting type constraints and usage patterns.The implementation successfully adds support for customer-provided typings, but it would be beneficial to document:
- Constraints on the
LiveMapTypeinterface- Example usage patterns with custom types
- Type safety considerations when dealing with object references
This documentation would help users understand how to properly define their custom types and handle type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(5 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/liveobjects.ts
- test/package/browser/template/src/ably.config.d.ts
- test/package/browser/template/src/index-liveobjects.ts
- test/package/browser/template/src/tsconfig.json
🔇 Additional comments (7)
src/plugins/liveobjects/livemap.ts (2)
49-49: LGTM! Good use of generic type parameter.
The generic type parameter T extends API.LiveMapType is well-defined and properly constrains the type to the API's LiveMapType interface.
73-76: Verify type safety of state object conversion.
While the generic type parameter is correctly used, we should ensure that the state object's structure matches the expected type T.
✅ Verification successful
Type safety is properly enforced in LiveMap's fromStateObject method
The implementation is type-safe because:
LiveMapTypeis well-defined as a mapped type with string keys to valid state values- The generic type parameter
Tis properly constrained withextends API.LiveMapType - The method is used correctly in the codebase, specifically in
LiveObjects.getRoot<T>() - The type hierarchy ensures that
StateObjectcan only contain valid map data matching the generic type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of fromStateObject to verify type safety
ast-grep --pattern 'LiveMap.fromStateObject<$_>($_)'
Length of output: 54
Script:
#!/bin/bash
# Let's try a different pattern to find usages of fromStateObject
rg -A 3 "LiveMap\.fromStateObject"
# Also search for LiveMap instantiation patterns
rg -A 3 "new LiveMap<"
# Let's also check the LiveMap class definition and its type parameter usage
ast-grep --pattern 'class LiveMap<$_> {
$$$
}'
Length of output: 677
Script:
#!/bin/bash
# Let's check the LiveMapType definition and its usage
rg -B 2 -A 2 "LiveMapType"
# Check the StateObject type definition
rg -B 2 -A 2 "type StateObject"
# Look for overrideWithStateObject implementation
ast-grep --pattern 'overrideWithStateObject($$$) {
$$$
}'
Length of output: 3108
ably.d.ts (5)
2032-2033: LGTM: Well-designed generic type signature
The updated getRoot method signature with generic type parameter provides type-safe access to the root object while maintaining backward compatibility through the DefaultRoot fallback.
2037-2042: LGTM: Well-designed extensibility point
The global LiveObjectsTypes interface provides a clean way for users to define their own types through declaration merging, with the unknown type offering maximum flexibility while maintaining type safety.
2044-2045: LGTM: Clear and recursive type definition
The LiveMapType effectively captures the hierarchical nature of LiveObjects, allowing for nested maps while maintaining type safety for scalar values and specialized types like LiveCounter.
2047-2055: Reference to previous review comment about using 'never' type
The suggestion to use 'never' instead of a string literal for invalid types is still applicable and would provide better compile-time errors.
2056-2057: LGTM: Well-structured interface with proper type constraints
The LiveMap interface effectively combines inheritance from LiveObject with type-safe access through the constrained generic get method.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
9da78a6 to
426fd8d
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
f513bc6 to
ad43ba9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
| @@ -2,6 +2,8 @@ import * as Ably from 'ably'; | |||
| import LiveObjects from 'ably/liveobjects'; | |||
| import { createSandboxAblyAPIKey } from './sandbox'; | |||
|
|
|||
| // TODO: check we can refer to types exported by LiveObjects plugin. | |||
There was a problem hiding this comment.
Do we need to address this?
There was a problem hiding this comment.
I don't think we can test types exported by the plugin literally, as it has any type by design (liveobjects.d.ts).
But instead we should just check that LiveObjects-related types are available in the ably export, stuff like Ably.LiveCounter for example.
There is already couple of type references that do just that, I will just add a separate line to test exported types explicitly.
There was a problem hiding this comment.
updated test comments and removed this TODO
| // so the next calls would fail. we only need to check that TypeScript types work | ||
| const foo: 'bar' = map?.get('foo')!; | ||
| const baz: 'qux' = map?.get('nestedMap')?.get('baz')!; | ||
|
|
||
| // check LiveMap subscription callback has correct TypeScript types | ||
| const { unsubscribe } = root.subscribe(({ update }) => { |
There was a problem hiding this comment.
Suggestion: could we add tighter types for the allowed key strings in the update object? (Okay to create a separate ticket for this)
There was a problem hiding this comment.
created https://ably.atlassian.net/browse/DTP-1094 for this
| @@ -29,10 +45,8 @@ globalThis.testAblyPackage = async function () { | |||
| }); | |||
| unsubscribe(); | |||
There was a problem hiding this comment.
shall we add a check for unsubscribeAll?
There was a problem hiding this comment.
it wasn't my intention to check every existing public method, as it can get quite bloaty, but instead to check some of the most interesting typing parts, e.g. customer provided typings and some tricky ones like subscription update objects.
the presence and functionality of the public methods is tested by regular unit/integration tests, not by this package test.
| // use nullish coalescing as we didn't actually create a counter object on the root, | ||
| // so the next calls would fail. we only need to check that TypeScript types work | ||
| // check LiveCounter type also behaves as expected | ||
| // same deal with nullish coalescing | ||
| counter?.value(); |
There was a problem hiding this comment.
should we check the return type of value() is a number by assigning to a typed variable here?
There was a problem hiding this comment.
added assigning to a typed variable
| @@ -13,9 +15,23 @@ globalThis.testAblyPackage = async function () { | |||
| const root = await liveObjects.getRoot(); | |||
There was a problem hiding this comment.
We should also add a check that the type parameter approach works, overriding the global type declared in the ably.config.d.ts
There was a problem hiding this comment.
ably.d.ts
Outdated
| @@ -2029,13 +2029,32 @@ export declare interface PushChannel { | |||
| * Enables the LiveObjects state to be subscribed to for a channel. | |||
| */ | |||
| export declare interface LiveObjects { | |||
| getRoot(): Promise<LiveMap>; | |||
| getRoot<T extends LiveMapType = DefaultRoot>(): Promise<LiveMap<T>>; | |||
| } | |||
|
|
|||
| // TODO: document public API for LiveObjects | |||
There was a problem hiding this comment.
yeah, initiall I thought I can postpone writing the docstrings until all the public API is in there (write API is missing rn), but we have the API reference test that expects all types in ably.d.ts to be documented, so I guess I'll do it now
There was a problem hiding this comment.
extracted read-only API documentation to a separate PR, see #1928, and updated this PR to also add documentation for new types.
| } | ||
|
|
||
| // LiveMap type representation of how it looks to the user. A mapping of string keys to scalar values (StateValue) or other Live Objects. | ||
| export type LiveMapType = { [key: string]: StateValue | LiveMap<LiveMapType> | LiveCounter | undefined }; |
There was a problem hiding this comment.
I just noticed that StateValue includes both the Buffer and Uint8Array types. Presumably the correct type is platform-dependent. Is there a way we can select the correct type from some type that denotes the platform? (I can appreciate that might be difficult)
There was a problem hiding this comment.
I just used the same approach we already had in the public types for the CipherKey and CipherKeyParams
Lines 2527 to 2531 in 18a2559
I think to make it platform dependant we would need to have two separate type declaration files, and configure their exports in package.json so node.js and browser environments get the appropriate one.
It is way beyond the scope of this PR and LiveObjects work, and probably has too little of an impact relative to the amount of work required to think about it right now
There was a problem hiding this comment.
updated comment for StateValue in https://github.com/ably/ably-js/compare/f921ce9147a0ad109c040343d0ef6c7838108bec..a1e29ff78a1140d332480a003dd547c5a32ef976
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
1e5634a to
5d72d44
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
ad43ba9 to
f921ce9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/plugins/liveobjects/livemap.ts (1)
102-104: Consider adding runtime type checking for object referencesWhile the type assertions are necessary, the objectId case might benefit from runtime type checking to ensure type safety when retrieving objects from the pool.
Consider adding a type guard:
if ('value' in data) { return data.value as T[TKey]; } else { - return this._liveObjects.getPool().get(data.objectId) as T[TKey]; + const obj = this._liveObjects.getPool().get(data.objectId); + if (!obj) return undefined; + // Consider adding runtime type checking here + return obj as T[TKey]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(6 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/liveobjects.ts
- test/package/browser/template/src/ably.config.d.ts
- test/package/browser/template/src/index-liveobjects.ts
- test/package/browser/template/src/tsconfig.json
🔇 Additional comments (5)
src/plugins/liveobjects/livemap.ts (4)
3-3: LGTM: Import statement properly added
The import for API types is correctly added and necessary for the new generic type support.
63-63: LGTM: Static factory method properly typed
The zeroValue method correctly implements the generic type parameter, maintaining type safety for factory creation.
73-74: LGTM: fromStateObject implementation maintains type safety
The method properly constructs and returns a typed LiveMap instance, with correct type inference in the constructor call.
86-87: LGTM: Get method signature ensures type safety
The method signature properly constrains the key type to be both a key of T and a string, ensuring type safety at compile time.
ably.d.ts (1)
2072-2073: Use 'never' instead of error string in 'DefaultRoot'
Replacing the error string with never in the DefaultRoot type ensures that TypeScript provides a compile-time error when users provide an incorrect type, leading to clearer and more immediate feedback.
Apply this diff to enforce compile-time errors:
export type DefaultRoot =
unknown extends LiveObjectsTypes['root']
? LiveMapType // no custom types provided; use the default untyped map representation for the root
: LiveObjectsTypes['root'] extends LiveMapType
- ? LiveObjectsTypes['root'] // "root" property exists, and it is of an expected type, we can use this interface for the root object in LiveObjects.
- : `Provided type definition for the "root" object in LiveObjectsTypes is not of an expected LiveMapType`;
+ ? LiveObjectsTypes['root'] // "root" was provided by the user and is of an expected type.
+ : never; // Invalid type provided for "root" in LiveObjectsTypes, enforcing a compile-time error.
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
5d72d44 to
028eec3
Compare
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
f921ce9 to
a1e29ff
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1922/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1922/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1922/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
test/package/browser/template/src/ably.config.d.ts (1)
3-15: Add JSDoc comments to document the type structure.While the type structure is well-defined, adding documentation would help users understand the purpose and constraints of each field.
Consider adding documentation like this:
+/** + * Example type demonstrating various LiveObjects capabilities. + * This type shows how to define a custom root structure with different field types. + */ type CustomRoot = { + /** Numeric value demonstration */ numberKey: number; + /** String value demonstration */ stringKey: string; // ... (similar for other fields) };Consider making the example more realistic.
The current literal types ('bar', 'qux') in the nested LiveMap structure might be too restrictive for a demonstration. Consider using more flexible types that better represent real-world usage.
mapKey?: LiveMap<{ - foo: 'bar'; + foo: string; nestedMap?: LiveMap<{ - baz: 'qux'; + baz: string; }>; }>;typedoc.json (1)
24-24: LGTM! Consider documenting this capability elsewhere.The exclusion of
LiveObjectsTypesfrom the API documentation is correct since it's meant to be user-defined. However, ensure that this capability is well-documented in the user guide or README to help users understand how to provide their own typings.test/package/browser/template/src/index-liveobjects.ts (2)
19-21: Consider adding negative test cases.The type assertion correctly validates that customer-provided types work. However, consider adding negative test cases to ensure type errors occur when invalid types are used.
Example addition:
// @ts-expect-error - Should error when using incorrect type const wrongRoot: Ably.LiveMap<{ invalidKey: Date }> = await liveObjects.getRoot();
33-39: Consider adding null checks for optional properties.While the type checking is good, consider adding explicit tests for null handling of optional properties to ensure type safety.
Example addition:
// Verify that optional properties are handled correctly const nullableMap: LiveObjectsTypes['root']['mapKey'] | undefined = root.get('optionalMapKey'); if (nullableMap) { const value: string = nullableMap.get('foo')!; }ably.d.ts (1)
2056-2058: Remove unnecessary 'export' keyword in global interface declarationIn a
declare globalblock, theexportkeyword is not needed. Interfaces withindeclare globalare automatically added to the global scope. Removing theexportkeyword avoids confusion.Apply this diff to remove the unnecessary
exportkeyword:declare global { - export interface LiveObjectsTypes { + interface LiveObjectsTypes { root: MyRoot; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(5 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)typedoc.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/liveobjects/liveobjects.ts
- test/package/browser/template/src/tsconfig.json
🔇 Additional comments (19)
test/package/browser/template/src/ably.config.d.ts (3)
1-1: LGTM! Imports are correctly typed and necessary.
The imports of LiveCounter and LiveMap from the 'ably' package are appropriate for the type definitions.
1-21: Verify compatibility with main type definitions.
The implementation looks good overall, but we should verify that this custom typing structure aligns with the core LiveObjects implementation.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Custom type definition aligns with core LiveObjects implementation
The implementation in ably.config.d.ts perfectly matches the expected structure for LiveObjects type definitions as shown in the main ably.d.ts file. Specifically:
- The global
LiveObjectsTypesinterface with arootproperty matches the documented pattern - The type structure using
LiveMapandLiveCounterfollows the core implementation's requirements - The implementation follows the exact same pattern shown in the official example in
ably.d.ts(lines 2046-2060)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for LiveObjects related types in the main definitions
ast-grep --pattern 'interface LiveObjects {
$$$
}'
# Look for existing usage patterns
rg -A 5 "getRoot|LiveObjects"
Length of output: 81513
17-21: LGTM! Global declaration is well-structured.
The global interface declaration follows TypeScript best practices and matches the PR objectives for customer-provided typings.
Let's verify there are no duplicate declarations of LiveObjectsTypes:
typedoc.json (1)
24-24: Verify if other interfaces should be excluded.
Let's check if there are other similar user-definable interfaces that should also be excluded from the documentation.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information. The results show that LiveObjectsTypes is the only global interface specifically designed for user customization of types in the LiveObjects feature. The comments in ably.d.ts clearly document this as the intended interface for users to define custom types, and the test files confirm this usage pattern.
No additional interfaces need to be excluded from documentation
The current exclusion of __global.LiveObjectsTypes is appropriate and sufficient. This interface is specifically designed as the sole extension point for user-defined types in LiveObjects, as evidenced by the documentation and usage patterns in the codebase. No other similar user-definable interfaces were found that would warrant exclusion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other global interfaces or types that might need to be excluded
# from documentation, particularly those meant for user customization.
# Look for global interface declarations
ast-grep --pattern 'declare global {
interface $_ {
$$$
}
}'
# Look for comments containing keywords related to user customization
rg -i "user.*define|custom.*type|extend.*interface" --type ts
Length of output: 1046
test/package/browser/template/src/index-liveobjects.ts (4)
3-3: LGTM! Type definitions are clear and purposeful.
The import of CustomRoot and definition of ExplicitRootType align well with the PR objective of supporting customer-provided typings.
Also applies to: 6-8
28-31: LGTM! Comprehensive type checking for primitive types.
The type assertions effectively validate that customer-provided types work correctly for numbers, strings, and booleans.
Line range hint 42-52: Add type checking for update payload properties.
The switch statement checks the update type, but consider adding type assertions for the update payload properties as well.
This aligns with the previous review comment about tighter types for update object keys (DTP-1094).
Example enhancement:
const { unsubscribe } = root.subscribe(({ update }) => {
switch (update.someKey) {
case 'removed':
// Type assertion for removed case
const removedValue: string = update.oldValue;
break;
case 'updated':
// Type assertion for updated case
const newValue: string = update.newValue;
break;
default:
const shouldExhaustAllTypes: never = update.someKey;
}
});62-64: LGTM! Explicit type override works as expected.
The test correctly validates that custom types can override the global LiveObjectsTypes interface, which is a key requirement from the PR objectives.
src/plugins/liveobjects/livemap.ts (6)
3-3: LGTM: Import statement is correctly added
The import of API types is necessary for the generic type parameter and follows the correct path structure.
63-64: LGTM: Static methods correctly propagate generic type
The static factory methods properly handle the generic type parameter, maintaining type safety through object creation.
Also applies to: 73-74
86-87: LGTM: Get method signature ensures type safety
The generic type constraint TKey extends keyof T & string correctly ensures that only valid keys can be accessed.
91-91: Remove unnecessary casts when returning undefined
The type assertions to T[TKey] are unnecessary when returning undefined.
Also applies to: 95-95
102-104: 🛠️ Refactor suggestion
Add runtime type checking for retrieved objects
Consider adding runtime type validation for objects retrieved from the pool to ensure type safety:
if ('value' in data) {
return data.value as T[TKey];
} else {
- return this._liveObjects.getPool().get(data.objectId) as T[TKey];
+ const obj = this._liveObjects.getPool().get(data.objectId);
+ if (!obj) {
+ return undefined as T[TKey];
+ }
+ // Consider adding runtime type validation here
+ return obj as T[TKey];
}Likely invalid or redundant comment.
49-49: Verify migration path for existing LiveMap usage
The addition of the generic type parameter is a breaking change that requires updates to existing code. We should ensure proper migration guidance is provided.
ably.d.ts (5)
2064-2064: Method signature update enhances type safety
The updated getRoot method signature introduces a generic type parameter, allowing users to specify the structure of the LiveObjects state. This improves type safety and flexibility.
2067-2074: Global interface declaration enables user-defined types
Declaring LiveObjectsTypes in the global scope allows users to define custom types for LiveObjects, enhancing extensibility and type safety.
2080-2080: 'LiveMapType' correctly defines the structure of LiveMap data
The LiveMapType type appropriately represents the data stored in a LiveMap, supporting scalar values, nested LiveMap, and LiveCounter instances.
Line range hint 2117-2117: Method signature update enhances type safety
The updated get method now uses a generic TKey constrained to the keys of T, improving type safety by ensuring that only valid keys can be used.
2096-2096:
Use 'never' instead of error string in 'DefaultRoot' to enforce compile-time errors
Replacing the error string with never in the DefaultRoot type ensures that TypeScript provides a compile-time error when users provide an incorrect type, leading to clearer and more immediate feedback.
Apply this diff to improve type safety:
export type DefaultRoot =
unknown extends LiveObjectsTypes['root']
? LiveMapType // no custom types provided; use the default untyped map representation for the root
: LiveObjectsTypes['root'] extends LiveMapType
? LiveObjectsTypes['root'] // "root" property exists, and it is of an expected type.
- : `Provided type definition for the "root" object in LiveObjectsTypes is not of an expected LiveMapType`;
+ : never; // Invalid type provided for "root" in LiveObjectsTypes, enforcing a compile-time error.
|
@mschristensen Addressed your feedback, fixed package and API reference tests. |
VeskeR
force-pushed
the
liveobjects/package-test
branch
from
028eec3 to
d59e167
Compare
Add globally defined `LiveObjectsTypes` to `intentionallyNotExported` [1] list for `typedoc`, as typedocs does not include documentation for the globally defined interfaces and complains that "__global.LiveObjectsTypes, defined in ./ably.d.ts, is referenced by ably.DefaultRoot but not included in the documentation." [1] https://typedoc.org/documents/Options.Validation.html#intentionallynotexported
VeskeR
force-pushed
the
DTP-963/liveobjects-customer-typings
branch
from
a1e29ff to
3938b63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
test/package/browser/template/src/index-liveobjects.ts (2)
19-21: Consider adding negative test cases for type safety.While the current type checks verify correct usage, consider adding negative test cases to ensure type errors are properly caught at compile time.
Example:
// @ts-expect-error - Should error when accessing non-existent key const shouldNotWork = root.get('nonExistentKey');
26-39: Well-structured type checking with good error handling.The comprehensive type checking covers all essential cases with proper runtime safety using nullish coalescing. The comments effectively explain the testing approach.
Consider adding explicit error case handling:
try { // @ts-expect-error - Should error when assigning wrong type const wrongType: number = root.get('stringKey'); } catch (error) { console.error('Expected type error caught:', error); }ably.d.ts (1)
2071-2073: Remove unnecessaryexportkeyword insidedeclare globalWithin the
declare globalblock, declarations are automatically added to the global scope, so theexportkeyword is unnecessary.Apply this diff:
declare global { /** * A globally defined interface that allows users to define custom types for LiveObjects. */ - export interface LiveObjectsTypes { + interface LiveObjectsTypes { [key: string]: unknown; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
ably.d.ts(1 hunks)src/plugins/liveobjects/livemap.ts(5 hunks)src/plugins/liveobjects/liveobjects.ts(1 hunks)test/package/browser/template/src/ably.config.d.ts(1 hunks)test/package/browser/template/src/index-liveobjects.ts(3 hunks)test/package/browser/template/src/tsconfig.json(1 hunks)typedoc.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/liveobjects/liveobjects.ts
- test/package/browser/template/src/ably.config.d.ts
- test/package/browser/template/src/tsconfig.json
- typedoc.json
🔇 Additional comments (9)
test/package/browser/template/src/index-liveobjects.ts (4)
3-3: LGTM! Clean type definition and import.
The new import and type definition are well-structured and serve their purpose for testing explicit type overrides.
Also applies to: 6-8
Line range hint 41-53: LGTM! Excellent exhaustive type checking.
The use of exhaustive type checking with never type is a great practice to ensure all update cases are handled.
54-60: LGTM! Comprehensive LiveCounter type validation.
Good coverage of LiveCounter functionality with proper type checking and cleanup.
62-64: LGTM! Verify type override behavior.
The explicit type override test is well-implemented. Let's verify that the type system correctly handles conflicts between global and explicit types.
✅ Verification successful
Type override behavior is correctly implemented and consistent
The verification shows that the type override functionality is properly implemented:
- The base type definition in
ably.d.tscorrectly supports generic type parameter:getRoot<T extends LiveMapType = DefaultRoot>() - The implementation in
liveobjects.tsmaintains the same type flexibility:getRoot<T extends API.LiveMapType = API.DefaultRoot>() - The test case demonstrates proper usage of explicit type override, consistent with the implementation
- No conflicting or inconsistent usage patterns were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of getRoot type overrides to ensure consistent usage
ast-grep --pattern 'liveObjects.getRoot<$_>()'
Length of output: 48
Script:
#!/bin/bash
# Search for getRoot usage patterns with a more flexible AST pattern
ast-grep --pattern 'getRoot<$$$>'
# Also search for any getRoot usage without type parameters
ast-grep --pattern 'getRoot()'
# Use ripgrep as a backup to find any getRoot references
rg "getRoot" -A 2
Length of output: 10646
src/plugins/liveobjects/livemap.ts (4)
3-3: LGTM: Import statement is correctly added
The import of API types is necessary for the new generic type parameter and follows the correct relative path structure.
63-64: LGTM: Static methods correctly implement generic type parameter
The static methods zeroValue and fromStateObject correctly implement and propagate the generic type parameter, maintaining type safety throughout object creation.
Also applies to: 73-74
86-87: Consider improving type safety for the get method
The method signature correctly constrains keys to be strings and exist in T. However, consider updating the return type to explicitly include undefined:
- get<TKey extends keyof T & string>(key: TKey): T[TKey] {
+ get<TKey extends keyof T & string>(key: TKey): T[TKey] | undefined {102-105: 🛠️ Refactor suggestion
Strengthen type safety for object references
When returning values from the pool, consider adding runtime type checking to ensure type safety:
if ('value' in data) {
return data.value as T[TKey];
} else {
- return this._liveObjects.getPool().get(data.objectId) as T[TKey];
+ const obj = this._liveObjects.getPool().get(data.objectId);
+ if (!obj) return undefined;
+ // Consider adding runtime type checking here
+ return obj as T[TKey];
}Likely invalid or redundant comment.
ably.d.ts (1)
2089-2097: Use 'never' instead of error string in DefaultRoot to enforce compile-time errors
Replacing the error string with never in the DefaultRoot type ensures that TypeScript provides a compile-time error when users provide an incorrect type, leading to clearer and more immediate feedback.
Apply this diff:
export type DefaultRoot =
unknown extends LiveObjectsTypes['root']
? LiveMapType // no custom types provided; use the default untyped map representation for the root
: LiveObjectsTypes['root'] extends LiveMapType
? LiveObjectsTypes['root'] // "root" property exists and is of an expected type.
- : `Provided type definition for the "root" object in LiveObjectsTypes is not of an expected LiveMapType`;
+ : never; // Invalid type provided for "root" in LiveObjectsTypes, enforcing a compile-time error.| @@ -45,7 +46,7 @@ export interface LiveMapUpdate extends LiveObjectUpdate { | |||
| update: { [keyName: string]: 'updated' | 'removed' }; | |||
| } | |||
|
|
|||
| export class LiveMap extends LiveObject<LiveMapData, LiveMapUpdate> { | |||
| export class LiveMap<T extends API.LiveMapType> extends LiveObject<LiveMapData, LiveMapUpdate> { | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking Change Impact Confirmed: Generic Type Parameter Addition
The verification reveals that this is indeed a breaking change with significant impact:
- Direct instantiations of
LiveMapwill need type parameters - The change affects core functionality in
liveobjects.tswheregetRoot<T>()returnsLiveMap<T> - Multiple internal implementations reference and extend
LiveMap - Type definitions in
ably.d.tsshow extensive usage ofLiveMapin the public API
Key affected areas:
src/plugins/liveobjects/liveobjects.ts: Core implementation usingLiveMap<T>src/plugins/liveobjects/syncliveobjectsdatapool.ts: Internal data management- Type definitions affecting public API contracts
- Test templates demonstrating expected usage patterns
🔗 Analysis chain
Breaking Change: Generic Type Parameter Addition
The addition of the generic type parameter T extends API.LiveMapType improves type safety but introduces a breaking change. Existing code that instantiates LiveMap directly will need to be updated to provide a type parameter.
Let's verify the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct LiveMap instantiations that need updates
echo "=== Direct LiveMap instantiations ==="
rg "new LiveMap\(" --type ts
# Search for LiveMap type references that need updates
echo "=== LiveMap type references ==="
rg "LiveMap[^<]" --type ts
Length of output: 8684
This PR is based on #1921, please review it first.
Users can provide their own typings for LiveObjects structure by declaring global
LiveObjectsTypesinterface:or alternatively by providing the state tree structure for the
liveObjects.getRootcall (useful if they user multiple channels with different liveobjects structure):Resolves DTP-963
Summary by CodeRabbit
Summary by CodeRabbit
New Features
LiveObjectsTypesfor customizable LiveObjects structures.ExplicitRootTypefor better root object structure definition.Bug Fixes
testAblyPackagefunction.Documentation
Chores