-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-3274): add type hinting for UpdateFilter #2842
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
Conversation
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 |
@dariakp I think I've address all the comments, we discussed having more schema examples that would result in mostly duplicating the |
8926fa4
to
768f5cb
Compare
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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:
tsd
tsd
to checking that the definitions are valid, since it is part of the linting process