这是indexloc提供的服务,不要输入任何密码
Skip to content

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

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

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 requested a review from dconeybe August 24, 2023 16:57
@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

@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.

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.

@tom-andersen tom-andersen force-pushed the tomandersen/updatedocParameterTypes branch from 7d2fa23 to 2ee882e Compare September 25, 2023 23:25
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.

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.

4 participants