Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DTP-963] Add support for customer provided typings for LiveObjects #1922
[DTP-963] Add support for customer provided typings for LiveObjects #1922
Changes from all commits
f4113d8
73c8abc
3938b63
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I just noticed that
StateValue
includes both theBuffer
andUint8Array
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used the same approach we already had in the public types for the
CipherKey
andCipherKeyParams
ably-js/ably.d.ts
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated comment for
StateValue
in https://github.com/ably/ably-js/compare/f921ce9147a0ad109c040343d0ef6c7838108bec..a1e29ff78a1140d332480a003dd547c5a32ef976Check warning on line 2483 in ably.d.ts
GitHub Actions / lint
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.
💡 Codebase verification
Breaking Change Impact Confirmed: Generic Type Parameter Addition
The verification reveals that this is indeed a breaking change with significant impact:
LiveMap
will need type parametersliveobjects.ts
wheregetRoot<T>()
returnsLiveMap<T>
LiveMap
ably.d.ts
show extensive usage ofLiveMap
in the public APIKey affected areas:
src/plugins/liveobjects/liveobjects.ts
: Core implementation usingLiveMap<T>
src/plugins/liveobjects/syncliveobjectsdatapool.ts
: Internal data management🔗 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 instantiatesLiveMap
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:
Length of output: 8684
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created https://ably.atlassian.net/browse/DTP-1094 for this
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.
shall we add a check for
unsubscribeAll
?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.
same on the counter
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.
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.