Skip to content

Conversation

@harrysolovay
Copy link

@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!

@vercel
Copy link

vercel bot commented Jun 28, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/iaincollins/next-auth-docs/jenx8uo3m
✅ Preview: https://next-auth-docs-git-fork-harrysolovay-main.iaincollins.vercel.app

@vercel vercel bot temporarily deployed to Preview June 28, 2020 16:12 Inactive
@harrysolovay
Copy link
Author

(Prompted by #220)

@vercel vercel bot temporarily deployed to Preview June 28, 2020 16:35 Inactive
@0ubbe
Copy link
Collaborator

0ubbe commented Jun 28, 2020

@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 main → milestone/ts-migration and open gradually small PRs against 🍉 ( for instance first one moving just src/client to TS )

I think @iaincollins has raised quite a few fair points here:

request and response objects from the services it integrates with are so varied and so conditionally and ambiguous that it gets very tuff to type them properly

and

having increased LoC and overhead of maintenance is not appealing. Re-writes can kill momentum on open source projects, because people have little incentive to push through the slog to get to work on the bits they feel deliver value.

@harrysolovay
Copy link
Author

@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.

@iaincollins
Copy link
Member

@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.

@harrysolovay
Copy link
Author

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?

@iaincollins
Copy link
Member

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.

@harrysolovay
Copy link
Author

That makes complete sense! Gonna close this out for now. Please ping me when you're ready to pick up on TS-related improvements :)

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