webauthn-open-source / fido2-lib

A node.js library for performing FIDO 2.0 / WebAuthn server functionality
https://webauthn.io
MIT License
394 stars 118 forks source link

Export types of missing MDS classes and functions #137

Closed gaizeror closed 10 months ago

gaizeror commented 10 months ago

main.d.ts missing MDS types, partially solves https://github.com/webauthn-open-source/fido2-lib/issues/136

JamesCullum commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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!

codecov-commenter commented 10 months ago

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.

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

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

orgaizer commented 10 months ago

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 commented 10 months ago

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.