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.

Sign up for GitHub

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

Jump to bottom

chore!: upgrade typescript to v4.1.5 #1592

Merged
merged 3 commits into from
Aug 20, 2021
Merged

chore!: upgrade typescript to v4.1.5 #1592

merged 3 commits into from
Aug 20, 2021

Conversation

thebrianchen
Copy link

No description provided.

@thebrianchen thebrianchen self-assigned this Aug 20, 2021
@thebrianchen thebrianchen requested review from a team as code owners August 20, 2021 15:52
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Aug 20, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 20, 2021
schmidt-sebastian
schmidt-sebastian reviewed Aug 20, 2021
View reviewed changes
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

I wonder if this needs to be chore! just to play it safe. This would trigger a major version bump.

dev/system-test/firestore.ts Outdated
@@ -444,7 +444,8 @@ describe('DocumentReference class', () => {
});

it('has set() method', () => {
const allSupportedTypesObject = {
/* eslint-disable @typescript-eslint/no-explicit-any */
const allSupportedTypesObject: {[field: string]: any} = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be unknown.

Copy link
Author

Choose a reason for hiding this comment

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

We need any since we access allSupportedTypesObject.pathValue.path on L478.

dev/test/index.ts Outdated
@@ -262,7 +262,8 @@ const allSupportedTypesInput = {
bytesValue: Buffer.from([0x1, 0x2]),
};

const allSupportedTypesOutput = {
/* eslint-disable @typescript-eslint/no-explicit-any */
const allSupportedTypesOutput: {[field: string]: any} = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be unknown.

Copy link
Author

Choose a reason for hiding this comment

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

We need any here since we access pathValue.formattedName on L680, which would would error since pathValue has unknown type.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can cast there if it happens just one. unknown as X is more type safe than any.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, done for both.

@schmidt-sebastian schmidt-sebastian removed their assignment Aug 20, 2021
@thebrianchen thebrianchen changed the title chore: upgrade typescript to v4.1.5 chore!: upgrade typescript to v4.1.5 Aug 20, 2021
schmidt-sebastian
schmidt-sebastian reviewed Aug 20, 2021
View reviewed changes
dev/test/index.ts Outdated
@@ -676,7 +676,8 @@ describe('snapshot_() method', () => {
// Deep Equal doesn't support matching instances of DocumentRefs, so we
// compare them manually and remove them from the resulting object.
expect(actualObject.get('pathValue').formattedName).to.equal(
expected.pathValue.formattedName
/* eslint-disable @typescript-eslint/no-explicit-any */
(expected.pathValue as any).formattedName
Copy link
Contributor

Choose a reason for hiding this comment

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

as DocumentReference

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for being hasty 😨

dev/system-test/firestore.ts Outdated
@@ -474,7 +474,8 @@ describe('DocumentReference class', () => {
.then(doc => {
const data = doc.data()!;
expect(data.pathValue.path).to.equal(
allSupportedTypesObject.pathValue.path
/* eslint-disable @typescript-eslint/no-explicit-any */
(allSupportedTypesObject.pathValue as any).path
Copy link
Contributor

Choose a reason for hiding this comment

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

as IDontKnowWhatTypeThisIsButWeCanFindOut

schmidt-sebastian
schmidt-sebastian approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

LGTM.

Let's not merge this until the follow-up PR is ready and we know how to make patch releases for prior versions.

@thebrianchen
Copy link
Author

LGTM.

Let's not merge this until the follow-up PR is ready and we know how to make patch releases for prior versions.

Yep, I'm merging this into a 4.1 feature branch that I'll add the other changes to.