Skip to content
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

Merged

Conversation

lukasjhan
Copy link
Member

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.

Signed-off-by: Lukas.J.Han <lukas.j.han@gmail.com>
Copy link

codecov bot commented Mar 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.41%. Comparing base (5906908) to head (60dcdfd).
Report is 2 commits behind head on next.

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.
📢 Have feedback on the report? Share it here.

@cre8
Copy link
Contributor

cre8 commented Mar 3, 2024

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.

@lukasjhan
Copy link
Member Author

@cre8 I experienced type-related build errors when building credo-ts.
Do you know of any other solutions to resolve these errors instead including explicit type?

@cre8
Copy link
Contributor

cre8 commented Mar 3, 2024

Right now we are not referencing lib.dom.ts in your configs, are we? The problem I had when I import JsonWebKey from e.g. node:crypto, then there are some type mismatching.

At this point I would prefer the way via lib.dom.ts since this is (as far as I know) natively supported in typescript.

@lukasjhan
Copy link
Member Author

Right now we are not referencing lib.dom.ts in your configs, are we? The problem I had when I import JsonWebKey from e.g. node:crypto, then there are some type mismatching.

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.

@lukasjhan
Copy link
Member Author

@cre8 I guess credo-ts wants us to include the type not natively.
openwallet-foundation/credo-ts#1787 (comment)

@TimoGlastra
Copy link
Contributor

@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)

@cre8
Copy link
Contributor

cre8 commented Mar 4, 2024

@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.

@lukasjhan
Copy link
Member Author

I made a sample repo 🙂 https://github.com/lukasjhan/ts-lib-checking
After I was digging more, I found out that it is okay if skipLibCheck in tsconfig.json is true.

@cre8
Copy link
Contributor

cre8 commented Mar 4, 2024

I made a sample repo 🙂 https://github.com/lukasjhan/ts-lib-checking After I was digging more, I found out that it is okay if skipLibCheck in tsconfig.json is true.

Ty Lukas :)

I will check the two options with its pros and cons:

  • define jsonwebkey in this project
  • pass it as type parameter

@cre8 cre8 self-assigned this Mar 4, 2024
cre8 added 2 commits March 4, 2024 12:46
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
@cre8
Copy link
Contributor

cre8 commented Mar 4, 2024

@lukasjhan I removed the RSA key type, the JsonWebKey interface alone should be fine

@lukasjhan lukasjhan merged commit 38ffb3c into openwallet-foundation-labs:next Mar 4, 2024
10 checks passed
cre8 added a commit to cre8/sd-jwt-js that referenced this pull request Mar 8, 2024
Signed-off-by: Lukas.J.Han <lukas.j.han@gmail.com>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Co-authored-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
cre8 added a commit that referenced this pull request Mar 8, 2024
Signed-off-by: Lukas.J.Han <lukas.j.han@gmail.com>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Co-authored-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants