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

feat: update auth documentation for app router #1894

Closed
wants to merge 6 commits into from

Conversation

akshithio
Copy link

Closes #1882

✅ Checklist

  • I have followed every step in the contributing guide (updated 2022-10-06).
  • The PR title follows the convention we established conventional-commit
  • I performed a functional test on my final commit

Changelog

Update sections about server-side fetching and use of tRPC + add warning about Client Provider section to highlight that it is relevant only to the use of the /pages router. Change one outdated hyperlink and add additional context in few areas for user clarity.


Screenshots

[Screenshots]

💯

Copy link

changeset-bot bot commented May 24, 2024

⚠️ No Changeset found

Latest commit: 33b6a9b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented May 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
create-t3-app ✅ Ready (Inspect) Visit Preview 💬 1 unresolved
✅ 1 resolved
Jun 2, 2024 7:43pm
t3-upgrade ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 2, 2024 7:43pm

Copy link

vercel bot commented May 24, 2024

@akshithio is attempting to deploy a commit to the t3-oss Team on Vercel.

A member of the Team first needs to authorize it.

@akshithio
Copy link
Author

i could add an additional commit to this pr to fix #1889 too maybe?

@@ -39,27 +46,33 @@ const User = () => {

## Retrieving session server-side
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should be split to 2 sections (pages and app)

the old pages setup is still relevant afaik?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure i understand? The pages setup is still very relevant but i thought a warning might be a good call. In theory, the server-side retrieval section serves as the "other section" that explains how auth works in app router situations.

Copy link
Member

Choose a reason for hiding this comment

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

you removed the gSSP part of the docs explaining how to retrieve session on the server in pages router.

Copy link
Author

Choose a reason for hiding this comment

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

I understand what you mean now.

Copy link
Member

@juliusmarminge juliusmarminge left a comment

Choose a reason for hiding this comment

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

Still a bit hesitant about removing the gSSP pattern as there's been some issues reporting invalid usage and these docs sort of fixed that.

You could add a toggle for this session like #1900 maybe?

We should have a more global switch later, but for now having a tab switch in individual places where it makes sense will have to be good enough 😅

@akshithio
Copy link
Author

akshithio commented May 26, 2024

Did not see that PR before! Seems like an easy enough change, I'll do that by tomorrow! Should I also add another commit into this same pr to fix #1889 because it's a similar sort of issue or would you prefer I open a different pr for that after this is closed?

@juliusmarminge
Copy link
Member

juliusmarminge commented May 26, 2024

Should I also add another commit into this same pr to fix #1889 because it's a similar sort of issue or would you prefer I open a different pr for that after this is closed?

Sure! Whatever is easiest for you.

@akshithio
Copy link
Author

akshithio commented May 27, 2024

I think I'd have to add an additional dependency in the form @mdx-js/react and @mdx-js/runtime since unlike #1900 where there is only one edit at the top of the page, there's both a continuous series of edits for this specific page and a few of them include code snippets so I'd have to pass down some .mdx from the component to the mark down file. Does that seem ok or am i going wrong somewhere?

p.s. i have negligible experience working with either mdx or astro, please let me know if there's something obvious i'm missing.

@juliusmarminge
Copy link
Member

you can make a react component that conditionally renders children based on some condition

so

<PagesOnly>
  Use gSSP...
</PagesOnly>
<AppOnly>
  Await in an RSC...
</AppOnly>

@juliusmarminge
Copy link
Member

The trpc stuff was fixed in ff32e08

Feel free to reopen for the auth changes if you get the time for it in the future

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.

feat: NextAuth documentation update for app router
2 participants