Skip to content

Conversation

@dhensby
Copy link
Contributor

@dhensby dhensby commented Jul 25, 2024

  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 dhensby force-pushed the pulls/typings branch 2 times, most recently from ac39f74 to 631cf6e Compare July 28, 2024 09:27
Copy link
Member

@jorenvandeweyer jorenvandeweyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dhensby
Copy link
Contributor Author

dhensby commented Jul 30, 2024

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
Copy link
Member

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 🙈

@dhensby
Copy link
Contributor Author

dhensby commented Jul 31, 2024

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
Copy link
Member

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

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.

3 participants