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(firestore): Version update @google-cloud/firestore to 5.x #1525

Merged
merged 3 commits into from
Jan 17, 2022

Conversation

makibishi0212
Copy link
Contributor

This PR resolves #1524

An update to typescript is also included in the PR to resolve the compilation error that occurs in Google-cloud/firestore.

@makibishi0212
Copy link
Contributor Author

Sorry, I didn't sign the CLA, I signed it.

@lahirumaramba lahirumaramba self-requested a review January 4, 2022 17:15
@lahirumaramba lahirumaramba self-assigned this Jan 4, 2022
@lahirumaramba
Copy link
Member

Hi @makibishi0212 Thank you for the contribution! Were you able to build and run unit tests locally for this change? It looks like our CIs are failing.

@hiranya911
Copy link
Contributor

It also seems Firestore 5.0 has breaking changes. Might want to be careful with our own semver here.

@makibishi0212
Copy link
Contributor Author

@lahirumaramba Thank you for review this PR!

Were you able to build and run unit tests locally for this change? It looks like our CIs are failing.

I checked unit test(by npm run test:unit) and integration test(by npm run test:integration -- --updateRules) on local environment. But as you mentioned, CI is failed, I will check the cause and fix it.

@makibishi0212
Copy link
Contributor Author

@lahirumaramba
I fixed the lint error caused by the typeScript version update, and confirmed that npm run lint will pass. Could you check it again with CI?

@makibishi0212
Copy link
Contributor Author

I'm sorry for repeating myself. Updated firebase-admin.api.md to deal with api-extractor error in CI.
This update reflects the addition of exports due to the upgrade of @google-cloud/firestore.

@lahirumaramba
Copy link
Member

@makibishi0212 Thank you for updating the PR!

It appears that a change introduced to tuples in TS 4 breaks the version of typescript-eslint we use. This is why you had to replace tuple types to arrays. While this is a reasonable immediate fix, I think we should try and come up with a proper solution here.

I suggest we update typescript, typescript-eslint/eslint-plugin, and @typescript-eslint/parser, which requires updating eslint, and then updating firestore as the last step. I think we should do this incrementally in separate PRs.

Let's keep this PR for its original purpose, which is to update firestore. Let me prepare the changes to update eslint and typescript first. Once the changes are in, you should be able to rebase this PR and keep the firestore specific updates.

Also, it seems like the updates to firebase-admin.api.md are associated with a different user account. Please see below.

82e16d9 PR Opener: @makibishi0212
82e16d9 Author: <kik******ngo​@kikuchikengonoMacBook-Pro.local>

To fix this, you could try and sign the CLA from the second account as well, or try to change the author of the last commit:

git commit --amend --author="John Doe <john@doe.org>"

Thank you again for your contribution! Let me know if you have any questions.

@makibishi0212
Copy link
Contributor Author

@lahirumaramba Thanks for the review.

I suggest we update typescript, typescript-eslint/eslint-plugin, and @typescript-eslint/parser, which requires updating eslint, and then updating firestore as the last step. I think we should do this incrementally in separate PRs.

I understand about the phased update here. Indeed, this change was an ad-hoc fix to an error in eslint, which is not a good change.

After the eslint and typescript updates are merged, I will rebase this PR and change it to update only the firestore version and firebase-admin.api.md .

And I will also address the author issue.

@lahirumaramba
Copy link
Member

@makibishi0212 The ESLint #1540 and TS #1541 updates are now merged. Please rebase this PR when you get a chance.
Thank you!

@makibishi0212
Copy link
Contributor Author

@lahirumaramba Thanks for the ESLint and TS updates!
I fixed PR so that only updates related to firestore version upgrades.

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@lahirumaramba lahirumaramba merged commit 1c939ff into firebase:master Jan 17, 2022
@lahirumaramba lahirumaramba changed the title version update @google-cloud/firestore to 5.x fix(firestore): Version update @google-cloud/firestore to 5.x Jan 17, 2022
@lahirumaramba lahirumaramba changed the title fix(firestore): Version update @google-cloud/firestore to 5.x feat(firestore): Version update @google-cloud/firestore to 5.x Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Firestore] Update dependency to 5.x
3 participants