-
Notifications
You must be signed in to change notification settings - Fork 389
Update SignOptions to fix a type error #1436
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
afe7cd1
to
2b40284
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.
Workaround looks ok with a couple of suggestions. But I'm wondering if we should downgrade the faulty dependency instead.
@@ -309,3 +315,12 @@ export const messaging = { | |||
dryRun: true, | |||
}, | |||
}; | |||
|
|||
function copyAlgorithmToHeader(options: SignOptions, overrides?: object): SignOptions { |
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.
We might be able to type overrides as Partial<SignOptions>
. That should make some of the type casts go away.
function copyAlgorithmToHeader(options: SignOptions, overrides?: object): SignOptions { | ||
// The following allows us to override the alg in header without replacing the kid. | ||
if (validator.isNonNullObject(overrides) && validator.isNonNullObject(options.header) && | ||
Object.prototype.hasOwnProperty.call(overrides, 'algorithm')) { |
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.
Just overrides.hasOwnProperty()
?
@@ -77,7 +77,7 @@ | |||
"@types/chai": "^4.0.0", | |||
"@types/chai-as-promised": "^7.1.0", | |||
"@types/firebase-token-generator": "^2.0.28", | |||
"@types/jsonwebtoken": "^8.5.0", | |||
"@types/jsonwebtoken": "^8.5.5", |
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'm wondering if we should just downgrade and pin this dependency to v8.5.1. Implementing all these workarounds to deal with a badly versioned dependency doesn't sound very good to me. WDYT?
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.
Agreed. Created a new PR to pin the dependency. I will close this for now.
Decided to pin the dependency to 8.5.1 for now #1438. Closing this PR. |
In
@types/[email protected]
the data type of the propertyheader
was changed fromheader: object
toheader: JWTHeader
. This results in type errors in our test build. This PR fixes it by setting thealg
inheader
and removing the optionalalgorithm
from the top level.