Skip to content
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

feat!: Fix the UpdateData incorrect parameter type issue #1887

Merged
merged 28 commits into from
Sep 27, 2023

Conversation

tom-andersen
Copy link
Contributor

@tom-andersen tom-andersen commented Aug 23, 2023

@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: firestore Issues related to the googleapis/nodejs-firestore API. labels Aug 23, 2023
@tom-andersen tom-andersen changed the title Tomandersen/updatedoc parameter types feat: Fix the UpdateData incorrect parameter type issue Aug 24, 2023
@tom-andersen tom-andersen marked this pull request as ready for review August 24, 2023 16:57
@tom-andersen tom-andersen requested review from a team as code owners August 24, 2023 16:57
@MarkDuckworth MarkDuckworth changed the title feat: Fix the UpdateData incorrect parameter type issue feat!: Fix the UpdateData incorrect parameter type issue Aug 30, 2023
@MarkDuckworth
Copy link
Contributor

I updated the name to feat!:... to indicate that this is a breaking change.

@tom-andersen tom-andersen changed the title feat!: Fix the UpdateData incorrect parameter type issue feat: Fix the UpdateData incorrect parameter type issue Aug 31, 2023
@tom-andersen tom-andersen changed the title feat: Fix the UpdateData incorrect parameter type issue feat!: Fix the UpdateData incorrect parameter type issue Aug 31, 2023
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some pervasive updates required for this PR. Any function that takes a DocumentReference, CollectionReference, etc. must have its template parameter updated to take <AppModelType, DbModelType extends DocumentData>. For example, BulkWriter.set() and BulkWriter.delete(). This includes function signatures that use unknown as the type for the first type parameter.

Right now this isn't showing up as a compilation error because nodejs-firestore depends on TypeScript 4.7.4; however, if you switch it to 5.1.3 in package.json you get 536 build errors when running npm install. This is because the newer version of TypeScript does more rigorous type checking, complaining about incompatible type usages that the older version did not detect.

These build errors should be fixed so that we don't break customers who are using the newer versions of TypeScript. Note that this had to be done in the web sdk too.

If you're using IntelliJ you can select the newer version of TypeScript and watch all of the red squigglies show up :)

@dconeybe dconeybe assigned tom-andersen and unassigned dconeybe Sep 6, 2023
Copy link
Contributor Author

@tom-andersen tom-andersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might need to be some more discussion on the right approach. I have a feeling we might disagree on some of the changes. There are cases where we want to use any and unknown, and those discussions might be better handled over GVC.

dev/src/bulk-writer.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/test/document.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
types/firestore.d.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
types/firestore.d.ts Show resolved Hide resolved
types/firestore.d.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
types/firestore.d.ts Show resolved Hide resolved
types/firestore.d.ts Show resolved Hide resolved
types/firestore.d.ts Outdated Show resolved Hide resolved
dev/src/bulk-writer.ts Outdated Show resolved Hide resolved
dev/src/document-reader.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Soooooo close! Just a few very minor nits.

types/firestore.d.ts Outdated Show resolved Hide resolved
dev/src/reference.ts Outdated Show resolved Hide resolved
dev/src/watch.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more tiny comments.

types/firestore.d.ts Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
types/firestore.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test coverage for calling FirebaseFirestore.getAll() with document references that have type converters, or pin the type parameters back to DocumentData, as it was before this PR.

@dconeybe
Copy link
Contributor

Please add test coverage for calling FirebaseFirestore.getAll() with document references that have type converters, or pin the type parameters back to DocumentData, as it was before this PR.

If you choose to pin the parameters, ONLY pin the types in firestore.d.ts; in index.ts, leave the signature as-is. It is true that they are somewhat out of sync, but this out-of-syncness reduces the diff of this PR and keeps it focussed.

@MarkDuckworth MarkDuckworth merged commit 0afadef into main Sep 27, 2023
17 checks passed
@MarkDuckworth MarkDuckworth deleted the tomandersen/updatedocParameterTypes branch September 27, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants