search

Nekonyx / next-auth-steam

Steam authentication provider for next-auth
70 stars 18 forks source link

SteamProvider rewrite #7

Closed alexevladgabriel closed 1 year ago

Nekonyx commented 1 year ago

Hey, thanks for the contribution.

The only thing I noticed about this PR is that the colors for the light theme have been fixed. However, the rest of the code suggestions are terrible.

If you can explain why these changes are made and what good they do for the package, then I'll change my mind about closing this PR.

alexevladgabriel commented 1 year ago
  1. You are not using the const in multiple files and the use of it in NextJS App Route is useless.
  2. Is more readable an easy to find them in the same file, as the example provided by Next Auth from Discord Provider.
  3. Typescript errors you have commented are now removed due to fixing the types correctly.
  4. Removed steamid from TokenSet, to not be required to create a database field in Prisma. Solving the #6 problem I talked about last days. The steamid being saved in id_token field in database, a better case would be saving it in session_state, not sure about it.
Nekonyx commented 1 year ago
  1. Constants are better left as they are. Provider ID or other details like EMAIL_DOMAIN may change in the future, for example if next-auth itself makes Steam provider support and this will conflict with the Provider ID.
  2. It would be much better to decompose entities by their purpose into separate files. We know that constants.ts contains constants, we know that steam.ts contains the provider itself, and so on.
  3. This is a useful change, I'll make it ASAP.
  4. I honestly don't really understand what TokenSet is for. Maybe I should really put SteamID in id_token instead of steamId.