-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[KeyVault] - Update API Extractor version #17702
Conversation
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.
Everything looks good except for some type mangling
@@ -218,6 +221,7 @@ export interface JsonWebKey { | |||
x?: Uint8Array; | |||
y?: Uint8Array; | |||
} | |||
export { JsonWebKey_2 as JsonWebKey } |
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.
this is bizarre to me
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.
It's not a bug, but it is the reason #9410 is stalled... I believe this is the change that introduced this microsoft/rushstack#1767
My understanding is that names that will conflict / shadow declarations in dom.d.ts, etc will be renamed as such
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.
But it's super pervasive - any clash will cause this noise...
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.
https://github.com/microsoft/TypeScript/blob/cec2fda9a53620dc545a2c4d7b0156446ab145b4/lib/lib.webworker.d.ts looks like it's getting declared here, which makes me wonder if this is a common browser type, should we even be exporting this? Or simply declaring a shim?
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.
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.
It's unfortunately not that simple - this JsonWebKey type that we declare here is not conformant with IETF standard for JWK... I believe since the full set is all optional the difference for us is keyOps
instead of key_ops
... other languages have more differences but for us I think that's what we would need.
But we already GA'd with this shape...
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.
since this is just the interface name, can we maybe rename it so it doesn't conflict anymore, but still export the conflict as deprecated alias?
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 can, sure. But I do want to call out that if we want to upgrade api-extractor for everyone we will keep bumping into this. Some are already showing this issue (like Error_2, Location_2, etc) so I'm not sure what the answer would be globally. Any thoughts?
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.
Created #17710 to address this
type KeyType_2 = string; | ||
export { KeyType_2 as KeyType } |
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.
this has to be some kind of bug, right?
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.
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.
Speaking of differences - KeyType for us does not mean the same as keyType in node crypto - for us it's the kty
(EC, RSA, etc). I think globally it's defined as "private" | "public" | "secret"
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.
This makes sense to me. Thank you for making the follow-up issue.
Closing as we did this as part of #17917 |
What
Why
As part of #9410 we are upgrading API Extractor piecemeal. We do need to have everyone upgraded to a later version
in order to support
import type
/export type
statements which will eventually enable us to re-export OTel's APIs