-
Notifications
You must be signed in to change notification settings - Fork 13
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: add jwk type #130
feat: add jwk type #130
Conversation
Signed-off-by: Lukas.J.Han <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #130 +/- ##
==========================================
+ Coverage 97.38% 97.41% +0.03%
==========================================
Files 23 23
Lines 1876 1899 +23
Branches 272 273 +1
==========================================
+ Hits 1827 1850 +23
Misses 49 49 ☔ View full report in Codecov by Sentry. |
I would not include it as a type. as you mentioned it's already included in the jsdom type, together with all the other key types like rsa, ec, etc. So to keep our type files smaller and clean I would not include it. |
@cre8 I experienced type-related build errors when building credo-ts. |
Right now we are not referencing At this point I would prefer the way via lib.dom.ts since this is (as far as I know) natively supported in typescript. |
That's good point :) I should find the way credo can use jsonwebkey that natively supported in typescript. |
@cre8 I guess credo-ts wants us to include the type not natively. |
@cre8 it seems a bit overkill to me to depend on lib dom just for JsonWebKey, and i also don't think it's needed to make the sd-jwt json web key type as complex as the lib dom one (i think only a few fields need to be defined and the rest can be typed as unknown). Including lib dom can cause issues with global types in React Native or Node.JS, so it doesn't make sense to me to depend on browser types in an environment agnostic library. Not adding lib.dom will also make it easier for people to add sd-jwt to their project, as they don't need to add lib dom (which will add a lot of globals which are only available in the browser) |
@TimoGlastra I will check the credo case, because when implementing the veramo plugin I did not need to include the dom lib. And our libs got compiled on node 16,18,20 with the latest typescript version. Instead of defining the the jsonwebkey type in this lib, we could also pass the type as a parameter so every environment is able to use the required one. |
I made a sample repo 🙂 https://github.com/lukasjhan/ts-lib-checking |
Ty Lukas :) I will check the two options with its pros and cons:
|
Signed-off-by: Mirko Mollik <[email protected]>
Signed-off-by: Mirko Mollik <[email protected]>
@lukasjhan I removed the RSA key type, the |
Signed-off-by: Lukas.J.Han <[email protected]> Signed-off-by: Mirko Mollik <[email protected]> Co-authored-by: Mirko Mollik <[email protected]> Signed-off-by: Mirko Mollik <[email protected]>
Signed-off-by: Lukas.J.Han <[email protected]> Signed-off-by: Mirko Mollik <[email protected]> Co-authored-by: Mirko Mollik <[email protected]> Signed-off-by: Mirko Mollik <[email protected]>
JsonWebKey type is in lib.dom.d.ts but some project might not want to include it tsconfig.json.
When I was trying to build credo-ts, It failed due to this. So I added it to our type file.