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(NODE-3274): add type hinting for UpdateFilter #2842

Merged
merged 5 commits into from
Jun 28, 2021
Merged

Conversation

nbbeeken
Copy link
Contributor

This brings in type checking / hinting for UpdateFilter. The tests in helper_types.test-d.ts and at the top of updateX.test-d.ts should cover all the new subtypes that make up the UpdateFilter type.

Additional changes:

  • Updated tsd
    • Had to fix type errors picked up by tsd, writeConcern not defining defaults, deps.ts exports not correctly union-ed and then later narrowed
  • Added running tsd to checking that the definitions are valid, since it is part of the linting process

@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 16, 2021
@dariakp dariakp self-requested a review June 16, 2021 16:58
test/types/community/collection/bulkWrite.test-d.ts Outdated Show resolved Hide resolved
src/write_concern.ts Show resolved Hide resolved
test/types/helper_types.test-d.ts Outdated Show resolved Hide resolved
test/types/helper_types.test-d.ts Show resolved Hide resolved
test/types/helper_types.test-d.ts Outdated Show resolved Hide resolved
test/types/helper_types.test-d.ts Outdated Show resolved Hide resolved
test/types/helper_types.test-d.ts Outdated Show resolved Hide resolved
test/types/helper_types.test-d.ts Outdated Show resolved Hide resolved
src/mongo_types.ts Outdated Show resolved Hide resolved
@dariakp
Copy link
Contributor

dariakp commented Jun 16, 2021

looks like tsc actually doesn't like the lack of typing on write concern; other things we discussed fixing is making sure the update tests use 2 schemas: one strictly defined and one that has a key with type; we want to make sure we clearly identify what we do and don't expect to be assignable in each case

@nbbeeken
Copy link
Contributor Author

@dariakp I think I've address all the comments, we discussed having more schema examples that would result in mostly duplicating the test/types/community/collection/updateX.test-d.ts. That isn't here because the result of looking into indexed schemas was that there isn't a way to preserve the key's narrowing. We can talk further offline, but this refers to the last example in test/types/helper_types.test-d.ts

@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 24, 2021
@dariakp dariakp self-requested a review June 24, 2021 14:37
@dariakp dariakp marked this pull request as ready for review June 24, 2021 14:40
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -71,6 +71,7 @@ function makeKerberosClient(authContext: AuthContext, callback: Callback<Kerbero
if ('kModuleError' in Kerberos) {
return callback(Kerberos['kModuleError']);
}
const { initializeClient } = Kerberos;
Copy link
Contributor

Choose a reason for hiding this comment

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

What motivated pulling this out vs leaving it as Kerberos.initializeClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cus Typescript! tsd has its own version of TS which was a version newer and these lines were throwing errors. You'll notice the deps.ts file needed changes to make the types more accurate. Pulling out the function here after you've narrowed the type makes it exist properly inside the callback below. Where as Kerberos.initializeClient has the error that the property potentially doesn't exist.

I can move the narrowing down into the callback too or just cast this problem away, open to diff solutions here

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine - slightly less readable, but not so bad as to necessitate a change.

@emadum emadum merged commit 05035eb into 4.0 Jun 28, 2021
@emadum emadum deleted the NODE-3274/update-type branch June 28, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
4 participants