Skip to content
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

[SDK-4499] Update to use the app router #131

Merged
merged 4 commits into from
Aug 25, 2023
Merged

[SDK-4499] Update to use the app router #131

merged 4 commits into from
Aug 25, 2023

Conversation

adamjmcgrath
Copy link
Contributor

See also auth0/docs#10293

@adamjmcgrath adamjmcgrath marked this pull request as ready for review August 23, 2023 15:08
@adamjmcgrath adamjmcgrath requested a review from a team as a code owner August 23, 2023 15:08
@@ -2,7 +2,7 @@
* @jest-environment node
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the dir structure of the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done e2220b5

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

With these changes, the sample app will no longer serve as an example of Next.js SDK + pages router, only with the app router, right?
Should we not showcase both, given that both are supported, and are likely being used in many real world apps due to incremental migration to the app directory?

@Widcket
Copy link
Contributor

Widcket commented Aug 24, 2023

Even the Next.js documentation features them both:

Screenshot 2023-08-24 at 11 50 50 PM

@adamjmcgrath
Copy link
Contributor Author

With these changes, the sample app will no longer serve as an example of Next.js SDK + pages router, only with the app router, right?

Correct

Should we not showcase both, given that both are supported, and are likely being used in many real world apps due to incremental migration to the app directory?

I disagree, there are many use cases not covered in this sample - including incremental migration. Adding support for pages would add too much bloat and would not represent a simple sample app. (we don't for example cover middleware in the sample either). For the swiss-army knife, cover all example - we have the example app in the repo

@Widcket
Copy link
Contributor

Widcket commented Aug 25, 2023

I don't mean for the sample app to cover all use cases (it's not meant for that) but to cover login, logout, and show user profile with both routers (same use cases covered in the QS, for which this is the companion sample app).
I understand if you consider that overkill, but given the relative newness of the app dir router, and that vercel itself has adapted their docs around both routers, maybe that's something to consider.

@Widcket
Copy link
Contributor

Widcket commented Aug 25, 2023

BTW, I don't think I mentioned that the sample app needs to cover all use cases 🤔 I wonder what gave you that impression.

@adamjmcgrath adamjmcgrath merged commit 997f233 into main Aug 25, 2023
4 checks passed
@adamjmcgrath adamjmcgrath deleted the app-dir branch August 25, 2023 08:44
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.

2 participants