-
Notifications
You must be signed in to change notification settings - Fork 4k
TypeScript Refactor #353
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
TypeScript Refactor #353
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/iaincollins/next-auth-docs/jenx8uo3m |
|
(Prompted by #220) |
|
@harrysolovay I think a different approach you could take on this is to work step by step. For instance, instead of opening a gigantic PR ( which ends very hard to review 😅 ) you could start branching off I think @iaincollins has raised quite a few fair points here:
and
|
|
@lluia I didn't mean to dismiss those concerns. Just thought it'd be good to have a place for us to begin hammering out these ideas (which is why this is in draft). That being said, I understand if this PR in itself comes across as rude (and for that I apologize). I can close this out if it's simply unproductive. |
|
@harrysolovay Thanks for raising this PR! I think it's super useful to have something like this as are reference and happy if you leave it open for other folks to look at. Even if we don't go merging a big PR to start with, having an idea of the scope of the task is really helpful and can help us carve up the work and work out how we'd tackle it. |
|
It was my pleasure! And a great way to get familiar with the codebase. There's definitely A LOT of difficulty in strictly typing so many different providers. I'm curious what you all think a phased approach looks like? What do we tackle first? |
|
Good question! My current thoughts are very much waiting until the End to End Tests (#298) are set up and to get the existing Type Definitions (#220) resolved and then be sure the core contributors are happy. So am not rushing to this as I think we need to address those first, and maybe addressing some functional issues / edge case bugs. We've also been talking about how examples can and should be structured and having a TS example once #220 is in makes sense. If we are ready to go forward from there, the idea from @lluia of tackling the client as the the first thing seems like a good starting point. If everyone in core is particular enthusiastic about it then we could move that forward sooner, as it's a discrete bit of work, but I want to see if everyone is on board first. |
|
That makes complete sense! Gonna close this out for now. Please ping me when you're ready to pick up on TS-related improvements :) |
@lluia @iaincollins I couldn't resist putting together this initial shot at refactoring to TS. I understand that it will take much time and testing to get this into sufficient shape. To set expectations: as is, there's a lot wrong with this PR. I've added
TODOs like a madman. And there's much work to be done on genericizing data sources, adapters and providers. All that said, hopefully you and/or other community members can work with me to get this looking decent. @LoriKarikari you expressed some interest in contributing to the TS refactor. Would you be open to helping out with this PR as well?I'll put together a list of questions that're most pressing if we're to get this PR out of draft.
Final shoutout: @iaincollins you've done a spectacular job at designing this––what I find to be––gorgeous auth API for NextJS. Really good work!