Optionally use strong type hierarchies for read outputs and write inputs.#50
Optionally use strong type hierarchies for read outputs and write inputs.#50
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the TypeScript typing capabilities of the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by adding strong typing for read and write operations using TypeScript generics. The use of advanced conditional types for path resolution and for deriving write shapes from read shapes is well-executed and will greatly improve the developer experience.
The changes are consistently applied across the NodeFire and Snapshot classes, propagating the new type information through methods like child, ref, parent, etc.
I have one critical point of feedback regarding the getNormalRawValue function, where an incorrect type cast can lead to runtime errors when dealing with arrays in data structures. Please see the specific comment for details.
Overall, this is a fantastic enhancement to the library's type safety.
src/nodefire.ts
Outdated
| function getNormalRawValue<T>(value: T): T { | ||
| if (_.isArray(value)) { | ||
| const normalValue = {}; | ||
| _.forEach(value, (item, key) => { | ||
| if (!_.isNil(item)) { | ||
| normalValue[key] = getNormalRawValue(item); | ||
| } | ||
| }); | ||
| value = normalValue; | ||
| value = normalValue as T; |
There was a problem hiding this comment.
The type signature for getNormalRawValue is <T>(value: T): T, but the implementation can return a different type than it received. If value is an array, it is converted to an object, but then cast back to T with value = normalValue as T; on line 918. This can lead to unexpected runtime errors if the caller expects an array.
For example:
interface MyData {
items: string[];
}
const ref = new NodeFire<MyData>(...);
const data = await ref.get(); // data is typed as MyData
// data.items is typed as string[] but is actually an object `{'0': 'a', '1': 'b'}` at runtime.
data.items.map(i => ...); // This will throw a runtime error: data.items.map is not a functionThis type cast is unsafe and subverts the type system. Since the array-to-object conversion is a documented feature of nodefire, the type signature should reflect this transformation to prevent runtime errors. You could achieve this with a Normalized<T> utility type that maps array types to object types.
A less ideal but simple fix would be to change the return type to any to signal the type is not guaranteed, but this would sacrifice the benefits of the new typings.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9093bc68bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/nodefire.ts
Outdated
| * error. The value returned is normalized, meaning arrays are converted to objects. | ||
| */ | ||
| get(options?: {timeout?: number, cache?: boolean}): Promise<StoredValue> { | ||
| get(options?: {timeout?: number, cache?: boolean}): Promise<Root> { |
There was a problem hiding this comment.
NodeFire.get is now typed as Promise<Root>, but Firebase returns null when the target path does not exist. In typed projects where Root is non-nullable (the common case), this creates a false non-null guarantee and can lead to runtime crashes after dereferencing the result from empty paths. The return type should include null (or Root should consistently encode nullability) to match actual DataSnapshot.val() semantics.
Useful? React with 👍 / 👎.
src/nodefire.ts
Outdated
| } | ||
|
|
||
| val(): Value { | ||
| val(): Root { |
There was a problem hiding this comment.
Snapshot.val() is declared to return Root, but callback snapshots can represent missing/deleted nodes and val() returns null in those cases. This again gives callers a non-null type that is not true at runtime, so listener code can compile and then fail when a node is removed or not yet created. The method’s return type should include null to preserve soundness.
Useful? React with 👍 / 👎.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski resolved 3 discussions.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on snoack).
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack reviewed 1 file and made 5 comments.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on pkaminski).
src/nodefire.ts line 251 at r2 (raw file):
child<const Paths extends readonly string[]>( path: string, scope: Scope, typePaths: Paths
What is going on here? So typePaths is unused in the implementation, what is the point of having it in this overload then? If it is just so that calling code can signal the type, can't it do so by passing it via the generic, i.e. child<Type_Goes_Here>(...)?
src/nodefire.ts line 265 at r2 (raw file):
/** * Creates a new NodeFire object on a child of this one, without interpolating the path. Useful * when the path may contain interpolation syntax that must be disregarded, and you've already
Looks like you accidentally removed a space here, misaligning the JSDoc comment.
src/nodefire.ts line 420 at r2 (raw file):
timeout?: number } ): Promise<ReadValue<Root> | null | undefined> {
Previously, the type returned by the updateFunction was the same generic returned by transaction itself.
src/nodefire.ts line 1026 at r2 (raw file):
type PathError<Message extends string> = { readonly [PATH_ERROR_BRAND]: Message; } & never;
& never collapses to never, so this is essentially quivalent to type PathError = never.
src/nodefire.ts line 1095 at r2 (raw file):
type UpdateShape<T> = StrictPartial<T> & {[path: string]: unknown | null};
unknown | null collapses to unknown.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski made 5 comments.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on snoack).
src/nodefire.ts line 251 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
What is going on here? So
typePathsis unused in the implementation, what is the point of having it in this overload then? If it is just so that calling code can signal the type, can't it do so by passing it via the generic, i.e.child<Type_Goes_Here>(...)?
This seemed like the most convenient way to be able to specify that ref will point to one of a set of paths. Could probably also do it by specifying the generic type directly but that's less readable IMO, especially as these will usually be multi-line lists.
src/nodefire.ts line 265 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
Looks like you accidentally removed a space here, misaligning the JSDoc comment.
Done.
src/nodefire.ts line 420 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
Previously, the type returned by the
updateFunctionwas the same generic returned bytransactionitself.
We didn't distinguish between read and write types before; I believe the new types are correct. The callback returns a write value (e.g., one with SERVER_TIMESTAMP), that the transaction then posts to Firebase, which in turn returns a snapshot of the written value with any placeholders resolved (i.e., a read value).
src/nodefire.ts line 1026 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
& nevercollapses tonever, so this is essentially quivalent totype PathError = never.
Indeed, Codex messed up and I didn't know enough to stop it. Should be fixed now, and it revealed a bunch of extra errors in the server as a bonus.
src/nodefire.ts line 1095 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
unknown | nullcollapses tounknown.
Right, that was a type bug. Fixed.
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack made 3 comments and resolved 2 discussions.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on pkaminski).
src/nodefire.ts line 251 at r2 (raw file):
Previously, pkaminski (Piotr Kaminski) wrote…
This seemed like the most convenient way to be able to specify that ref will point to one of a set of paths. Could probably also do it by specifying the generic type directly but that's less readable IMO, especially as these will usually be multi-line lists.
Isn't this specifically what the generic type syntax is for?
Looking at a real world example:
const enrollmentRef = db.child(
'users/{subtask.userKey}/enrollments/{subtask.type}',
scope,
['users/$/enrollments/public', 'users/$/enrollments/private', 'users/$/enrollments/personal']
);... would become ...
const enrollmentRef = db.child<[
'users/$/enrollments/public',
'users/$/enrollments/private',
'users/$/enrollments/personal'
]>('users/{subtask.userKey}/enrollments/{subtask.type}', scope);I don't think that is any less readable.
src/nodefire.ts line 420 at r2 (raw file):
Previously, pkaminski (Piotr Kaminski) wrote…
We didn't distinguish between read and write types before; I believe the new types are correct. The callback returns a write value (e.g., one with
SERVER_TIMESTAMP), that the transaction then posts to Firebase, which in turn returns a snapshot of the written value with any placeholders resolved (i.e., a read value).
But don't we lose inferability of the return value based on what the updateFunction returns? If the return type of updateFunction is lets say Foo, Promise<Foo> was inferred as return type of transaction before. I suppose there are edge cases like SERVER_TIMESTAMP though.
src/nodefire.ts line 1095 at r2 (raw file):
Previously, pkaminski (Piotr Kaminski) wrote…
Right, that was a type bug. Fixed.
{} | null collapses to {}. I'm not sure if it's possible to express in TypeScript "anything but undefined".
BTW, {} is equivalent to NonNullable<unknown> which you could use instead of silencing the ESLint error. But since you want to allow null here, you might have to use any.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski made 3 comments.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on snoack).
src/nodefire.ts line 251 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
Isn't this specifically what the generic type syntax is for?
Looking at a real world example:
const enrollmentRef = db.child( 'users/{subtask.userKey}/enrollments/{subtask.type}', scope, ['users/$/enrollments/public', 'users/$/enrollments/private', 'users/$/enrollments/personal'] );... would become ...
const enrollmentRef = db.child<[ 'users/$/enrollments/public', 'users/$/enrollments/private', 'users/$/enrollments/personal' ]>('users/{subtask.userKey}/enrollments/{subtask.type}', scope);I don't think that is any less readable.
I'm not a fan of of separating a function's arguments from the function name by a few lines of type clutter. The downside of my approach is that it's not clear whether the third argument is actually used for anything. Dunno, feels like 6 of one kind half dozen of the other; the only good thing is that this form is used super rarely so shouldn't matter much in the end.
src/nodefire.ts line 420 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
But don't we lose inferability of the return value based on what the
updateFunctionreturns? If the return type ofupdateFunctionis lets sayFoo,Promise<Foo>was inferred as return type oftransactionbefore. I suppose there are edge cases likeSERVER_TIMESTAMPthough.
The return value of transaction doesn't depend on what updateFunction returns; if the previous types implied this then they were wrong. transaction always returns the value of a snapshot at the given location (unless the transaction was aborted with undefined).
src/nodefire.ts line 1095 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
{} | nullcollapses to{}. I'm not sure if it's possible to express in TypeScript "anything but undefined".BTW,
{}is equivalent toNonNullable<unknown>which you could use instead of silencing the ESLint error. But since you want to allownullhere, you might have to useany.
Apparently {} | null doesn't collapse to {} with strictNullChecks: true turned on, which it is for all our projects.
I think {} communicates intent better than NonNullable<unknown> and suppressing the linter further clarifies that this type is very much intended.
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack made 1 comment and resolved 2 discussions.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on pkaminski).
src/nodefire.ts line 251 at r2 (raw file):
Previously, pkaminski (Piotr Kaminski) wrote…
I'm not a fan of of separating a function's arguments from the function name by a few lines of type clutter. The downside of my approach is that it's not clear whether the third argument is actually used for anything. Dunno, feels like 6 of one kind half dozen of the other; the only good thing is that this form is used super rarely so shouldn't matter much in the end.
If the concern is having too much boilerplate right after the function name, you can use a temporary type definition:
type EnrollmentPaths = ['users/$/enrollments/public', 'users/$/enrollments/private', 'users/$/enrollments/personal'];
const enrollmentRef = db.child<EnrollmentPaths>('users/{subtask.userKey}/enrollments/{subtask.type}', scope);Also the type wouldn't need to be an array to start with, and then it could be expressed more concise as template literal type:
type EnrollmentPath = `users/$/enrollments/${'public'|'private'|'personal'}`;The benefits I'm seeing here is:
- This is exactly what generics syntax is intended for in TypeScript, and adding a dummy argument just to have the type inferred feels like we are working against TypeScript.
- It makes it undoubtedly clear that these information are unused at run time.
- It keeps the generated JavaScript code small.
- The type does not need to be an array, but can leverage type unions and template literal types.
pkaminski
left a comment
pkaminski
left a comment
There was a problem hiding this comment.
@pkaminski made 1 comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on snoack).
src/nodefire.ts line 251 at r2 (raw file):
Previously, snoack (Sebastian Noack) wrote…
If the concern is having too much boilerplate right after the function name, you can use a temporary type definition:
type EnrollmentPaths = ['users/$/enrollments/public', 'users/$/enrollments/private', 'users/$/enrollments/personal']; const enrollmentRef = db.child<EnrollmentPaths>('users/{subtask.userKey}/enrollments/{subtask.type}', scope);Also the type wouldn't need to be an array to start with, and then it could be expressed more concise as template literal type:
type EnrollmentPath = `users/$/enrollments/${'public'|'private'|'personal'}`;The benefits I'm seeing here is:
- This is exactly what generics syntax is intended for in TypeScript, and adding a dummy argument just to have the type inferred feels like we are working against TypeScript.
- It makes it undoubtedly clear that these information are unused at run time.
- It keeps the generated JavaScript code small.
- The type does not need to be an array, but can leverage type unions and template literal types.
Brilliant, done!
snoack
left a comment
snoack
left a comment
There was a problem hiding this comment.
@snoack reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pkaminski).
2 participants
This change is