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

Export types of missing MDS classes and functions #137

Merged
merged 2 commits into from
Sep 17, 2023

Conversation

gaizeror
Copy link
Contributor

@gaizeror gaizeror commented Sep 1, 2023

main.d.ts missing MDS types, partially solves #136

@JamesCullum
Copy link
Member

Thanks for the contribution, it's more than welcome!

Can we do without a new external dependency? We've been working on slowly reducing dependencies where possible, so adding one only for types might offer alternative solutions. If it's only about static type definition, what would be the impact to copy them over?

I think the original idea behind the typescript was to only add the types for the common APIs. You mentioned that it only partly solves your request - what else would we need? I think the current goal is not to strive for 100%, but make it a pleasing experience for typescript authors.

@gaizeror
Copy link
Contributor Author

Hey @JamesCullum
I think that this import doesn't lead us to another direction regarding the dependencies since jose is already installed in the project and used.
When it comes to types, copying them over might cause issues on dependency upgrade. For example, if the next version of jose will include new claims in JWTPayload, or will remove some, it may cause run time error which could have been caught on development time with the correct type.

Having a main.d.ts which includes some types but not all is worse than not having any since it misleads the IDE. If main.d.ts exists, typescript "assumes" that these are all the types supported in this lib which makes the class interface to not match the actual interface.

My suggestion would be to either remove the file or maintain it with every change.

@JamesCullum
Copy link
Member

Thanks for the great feedback! Do you know where the additional deno-lock-file comes from? This is where my confusion was probably coming from.

@gaizeror
Copy link
Contributor Author

Actually I am not familiar with deno, all I know is that deno is deprecated.
I would consider converting / forking this repo to a TS project to serve both JS and TS projects.
As for the deno file, it was automatically generated somehow

Thanks for the great feedback! Do you know where the additional deno-lock-file comes from? This is where my confusion was probably coming from.

@JamesCullum
Copy link
Member

all I know is that deno is deprecated

I'm not sure where you've got that from, but I wasn't able to find any sources supporting that. Let's focus on the PR then. I'll remove the Deno.lock changes and merge then, as the changes otherwise are useful additions - thanks!

Reverse deno-lock changes
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (8d25430) 92.97% compared to head (c77edbf) 92.97%.
Report is 1 commits behind head on master.

❗ Current head c77edbf differs from pull request most recent head fda4ce2. Consider uploading reports for the commit fda4ce2 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #137   +/-   ##
=======================================
  Coverage   92.97%   92.97%           
=======================================
  Files          16       16           
  Lines        6007     6007           
=======================================
  Hits         5585     5585           
  Misses        422      422           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JamesCullum JamesCullum merged commit b31e86e into webauthn-open-source:master Sep 17, 2023
10 checks passed
@orgaizer
Copy link
Contributor

orgaizer commented Sep 17, 2023

all I know is that deno is deprecated

I'm not sure where you've got that from, but I wasn't able to find any sources supporting that. Let's focus on the PR then. I'll remove the Deno.lock changes and merge then, as the changes otherwise are useful additions - thanks!

Sorry, I might have been wrong about it.. Anyways I couldn't make the lib work with MDS v3 so far, I think it's not true that it supports MDS V3. Do you know if there is a guide/example for it?

@JamesCullum
Copy link
Member

I've seen a few PRs and unit tests about it, so I'm confident it should work. My own projects didn't use it, so I'd recommend searching around in our Github repository and issues for people with a similiar challenge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants