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: add Nextjs SSR example #49

Merged
merged 6 commits into from
May 23, 2023
Merged

feat: add Nextjs SSR example #49

merged 6 commits into from
May 23, 2023

Conversation

raynerljm
Copy link
Contributor

@raynerljm raynerljm commented May 15, 2023

Problem

Our SDK currently lacks example apps demonstrating how to use our SDK. This introduces two problems

  1. RPs do not have example apps following good practices to refer to
  2. We are not able to easily test SDK changes with an app (we need to do an npm link)

Closes #384

Solution

Added an example Next.js app using SSR and server components (Next.js 13.4 feature). It is hosted here.

Features:

  • Utilizes an in-memory store src/lib/store.js to store Session data (e.g. state, nonce, codeVerifier, etc.)

Tests

// TODO: ADD recording

@raynerljm raynerljm changed the title feat: add Nextjs SSR example (WIP) feat: add Nextjs SSR example May 15, 2023
@raynerljm raynerljm marked this pull request as draft May 15, 2023 10:25
Copy link
Contributor

@kwajiehao kwajiehao left a comment

Choose a reason for hiding this comment

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

Left some comments, but otherwise looks good to me!

I have 2 more points I wanted to talk about:

1. PKCE

What is the plan for this? I think it might be useful to include PKCE in this example, keen to hear your thoughts on how we planned to roll that out

2. Error-handling

I know we are using an in-memory data store, which makes sense. But currently if you complete the login flow, and then refresh the page, you get this error page:

Screenshot 2023-05-16 at 2 17 43 PM

I'm wondering if it would be more useful if we can render an error page instead with some information on why this is not showing / some other kind of error message.

What do you think?

examples/nextjs-ssr/README.md Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this file and add .DS_Store to our .gitignore file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup okay!

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, remove this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

examples/nextjs-ssr/README.md Show resolved Hide resolved
examples/nextjs-ssr/src/app/success/page.tsx Outdated Show resolved Hide resolved
examples/nextjs-ssr/src/app/api/auth-url/route.ts Outdated Show resolved Hide resolved
examples/nextjs-ssr/src/app/api/auth-url/route.ts Outdated Show resolved Hide resolved
examples/nextjs-ssr/README.md Outdated Show resolved Hide resolved
@raynerljm
Copy link
Contributor Author

@kwajiehao with regards to your 1st point - yes! when you reviewed it the example had yet to be updated with PKCE (that's why it's WIP) - but now I've updated it with PKCE

with regards to the 2nd point - the error messages are not showing up because we are using next start to run the server and thus it's treated as a prod environment as the error messages are being omitted. I've searched around and I'm not sure that Next.js allows us to run next start while changing NODE_ENV to development. we could instead put change the message to prompt users to check their terminal for the error instead - what do you think?

@PrawiraGenestonlia
Copy link
Contributor

with regards to the 2nd point - the error messages are not showing up because we are using next start to run the server and thus it's treated as a prod environment as the error messages are being omitted. I've searched around and I'm not sure that Next.js allows us to run next start while changing NODE_ENV to development. we could instead put change the message to prompt users to check their terminal for the error instead - what do you think?

This is next.js new features. It does not forward server error to FE through SSR unless we do special error boundary. Let's exclude this for this release because I don't think this is super important. Just some next.js shit features..

@kwajiehao
Copy link
Contributor

kwajiehao commented May 16, 2023

with regards to the 2nd point - the error messages are not showing up because we are using next start to run the server and thus it's treated as a prod environment as the error messages are being omitted. I've searched around and I'm not sure that Next.js allows us to run next start while changing NODE_ENV to development. we could instead put change the message to prompt users to check their terminal for the error instead - what do you think?

Saw the changes you made - they look good to me! Don't think we need to do anything more beyond that. Thanks @raynerljm

@raynerljm raynerljm changed the title (WIP) feat: add Nextjs SSR example feat: add Nextjs SSR example May 16, 2023
@raynerljm raynerljm marked this pull request as ready for review May 17, 2023 05:49
@raynerljm raynerljm force-pushed the feat/example-next-ssr branch from bc8af30 to e2fa907 Compare May 22, 2023 02:42
Copy link
Contributor

@kwajiehao kwajiehao left a comment

Choose a reason for hiding this comment

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

lgtm

@raynerljm raynerljm merged commit ac94159 into develop May 23, 2023
@raynerljm raynerljm deleted the feat/example-next-ssr branch May 23, 2023 08:13
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