-
Notifications
You must be signed in to change notification settings - Fork 20
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
Typescript improvements #2299
base: development/8.2
Are you sure you want to change the base?
Typescript improvements #2299
Conversation
Hello williamlardier,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
0b32365
to
28fd751
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.2 #2299 +/- ##
===================================================
+ Coverage 68.72% 68.75% +0.03%
===================================================
Files 216 216
Lines 17462 17454 -8
Branches 3646 3629 -17
===================================================
Hits 12000 12000
+ Misses 5458 5450 -8
Partials 4 4 ☔ View full report in Codecov by Sentry. |
lib/auth/auth.ts
Outdated
if (res.params.version === 2) { | ||
// @ts-ignore | ||
return vault!.authenticateV2Request(res.params, requestContexts, cb); | ||
res.params!.log = log; |
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.
do you know why Typescript is not able to infer the type here?
As far as I understand, extractParams()
return either AuthV2RequestParams, AuthV4RequestParams or AuthInfo. Since the AuthInfo case is handle, TS should be able to deduce this will be either AuthV2RequestParams or AuthV4RequestParams...
Maybe this is due to the "optional" params (params?
) in result of extractParams?
And the return type should be better type to either return an error without params, or params with null error: so that type deduction can work its magic, and we don't need type coercion...
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 updated the PR, I created a new type that allows TS to understand that params is defined if err
is null
.
value: newBucketMD, | ||
value: { | ||
...newBucketMD, | ||
quotaMax: new Long(newBucketMD.quotaMax || '0'), |
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.
should have an associated test, to ensure we can indeed properly create buckets with very large quotas
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.
Creating a bucket with a quota is not possible today, this is only to enforce the right type in the DB, as it fails the type validation. Having large quotas is already tested in the proper routes, and this Long
conversion is (indirectly) tested when we create buckets, when we test that we can properly pass a BucketInfo
object.
Do we still want such test?
581e44f
to
9e42492
Compare
I added a commit to fix the types of the loggers and fix a conflict with Vault.ts. |
2ce22e9
to
7e7674f
Compare
7e7674f
to
9925cc3
Compare
Improve TS:
_
prefix to define private variable in TS classes was removed: this is not following the TS standard. More file might still be affected.any
.