node-oauth / node-oauth2-server

🚀 The successor to oauthjs/oauth2-server. 🔒 Complete, compliant, maintained and well tested OAuth2 Server for node.js. Includes native async await and PKCE.
https://www.npmjs.com/package/@node-oauth/oauth2-server
MIT License
286 stars 39 forks source link

refactor(types): update types to be more concise #314

Closed dhensby closed 1 month ago

dhensby commented 1 month ago
  1. Don't use | undefined for props that are already declared optional (undefined is an implied value)
  2. prefer using the Record built-in type over object maps
dhensby commented 1 month ago

FYI: optional is not the same as undefined, by removing this, passing an undefined value is not allowed anymore.

I'm almost certain that's not right: https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgCoQM5mQbwFDKHIAOUA9sdGAJ4D8AXMllKAOYDceAvnngmSCzJIWRuiEBeXCXKUoNRgFcQAEwgxQEFci6cgA

This doesn't give a syntax error

jankapunkt commented 1 month ago

Just a question related to #309 - shouldn't scope for the authenticate handler now being

scope?: string | string[]

as the auth handler now supports both? Neither see this here, nor in the current master after merging #309. I hope I don't cause confusion with this question now :see_no_evil:

dhensby commented 1 month ago

It was off the back of this comment that I made the type just string[].

Basically the types reflect the ideal typings and not an enumeration of every single type that could be accepted. By limiting the type this way, we also make the upgrade path for those following the types easier if/when we remove the string support. It's also not a breaking change as it's the current typing, so we aren't introducing any new issues by leaving the typing like this.

jankapunkt commented 1 month ago

From my end it's ok but I'm only using a few of them for my IDE intellisense.