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

Use new @clerk/nextjs package in with-clerk example, update Next.js authentication docs #28906

Merged
merged 21 commits into from
Sep 17, 2021

Conversation

colinclerk
Copy link
Contributor

@colinclerk colinclerk commented Sep 8, 2021

Documentation / Examples

  • Make sure the linting passes

This PR makes two updates.

1. Update the with-clerk example to use a newly released @clerk/nextjs package

Previously, Clerk and Next.js required a combination of @clerk/clerk-react (for react) and @clerk/clerk-sdk-node (for api routes). We combined the functionality into a single, Next.js-specific package to provide a better developer experience around installation and imports.

2. Update the Next.js Authentication docs to add an example of API route authentication

Currently, the authentication docs only provide examples for adding authentication to Pages (both with static generation and server-side rendering). This PR adds an example for authenticating an API route.

@ijjk ijjk added type: documentation examples Issue was opened via the examples template. labels Sep 8, 2021
import { withSession } from '@clerk/nextjs/api'

// Use withSession to decorate `req` with a session object
export default withSession((req, res) => {
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this provider agnostic? We'd like to avoid picking favorites here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely. In general, I tried to use the precedent from the with-iron-session examples above - talking generically about the pattern but leveraging a specific example.

I just added a commit that obscures the import:
7a57cd4

Copy link
Member

Choose a reason for hiding this comment

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

It's still referencing Clerk below - I've tried to keep this provider agnostic, with iron session being an OSS library. Can we use that as the example and ensure the code snippet is reflected in the example?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@colinclerk colinclerk Sep 8, 2021

Choose a reason for hiding this comment

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

Ok, just pushed an update with the linked example and re-ran linters.

Lmk if there are any other issues!

132f958

@colinclerk
Copy link
Contributor Author

It's still referencing Clerk below though in this section - and is this a valid code snippet from with-iron-session?

There's a catch-22 here. Next.js doesn't provide a standardized interface for authentication providers to adhere to, so no two providers work alike.

So far, the Authentication docs have used generic copywriting about the available patterns, but specific examples from a single provider. The single provider used for the first two examples is with-iron-session, for example:

You can view this example in action. Check out the with-iron-session example to see how it works.

https://nextjs.org/docs/authentication#authenticating-statically-generated-pages

In adding an example about Authenticating API routes, I used a Clerk example, but tried to use the precedent from with-iron-session to make sure I wasn't crossing the line re: picking favorites. Specifically - I wrote generically about the pattern, but used a Clerk-specific example.

What do you think this is the best path forward here?

@braden-clerk
Copy link

Hey, taking this over! We switched the example to with-iron-session

Let me know if we need to make any more changes to this PR, thanks @leerob :)

@leerob
Copy link
Member

leerob commented Sep 16, 2021

If you'd like to get this merged faster, could we break up the example change and the docs change? The docs will still need some revisions.

@ijjk
Copy link
Member

ijjk commented Sep 16, 2021

Failing test suites

Commit: 132f958

test/integration/create-next-app/index.test.js

  • create next app > should allow example with GitHub URL
  • create next app > should allow example with GitHub URL and example-path
  • create next app > should use --example-path over the file path in the GitHub URL
  • create next app > should use npm as the package manager on supplying --use-npm with example
Expand output

● create next app › should allow example with GitHub URL

Command failed with exit code 1 (EPERM): node /home/runner/work/next.js/next.js/packages/create-next-app/dist/index.js github-app --example https://github.com/vercel/next-learn-starter/tree/master/navigate-between-pages-starter

  136 |       ).toBeTruthy()
  137 |
> 138 |       // Assert for dependencies specific to the typescript template
      |                         ^
  139 |       const pkgJSON = require(pkgJSONPath)
  140 |       expect(Object.keys(pkgJSON.dependencies)).toEqual([
  141 |         'next',

  at makeError (../node_modules/execa/lib/error.js:58:11)
  at handlePromise (../node_modules/execa/index.js:112:26)
  at integration/create-next-app/index.test.js:138:25
  at usingTempDir (integration/create-next-app/index.test.js:23:9)
  at Object.<anonymous> (integration/create-next-app/index.test.js:136:9)

● create next app › should allow example with GitHub URL and example-path

Command failed with exit code 1 (EPERM): node /home/runner/work/next.js/next.js/packages/create-next-app/dist/index.js github-example-path --example https://github.com/vercel/next-learn-starter/tree/master --example-path navigate-between-pages-starter

  153 |   })
  154 |
> 155 |   it('should allow example with GitHub URL', async () => {
      |                         ^
  156 |     await usingTempDir(async (cwd) => {
  157 |       const projectName = 'github-app'
  158 |       const res = await run(

  at makeError (../node_modules/execa/lib/error.js:58:11)
  at handlePromise (../node_modules/execa/index.js:112:26)
      at runMicrotasks (<anonymous>)
  at integration/create-next-app/index.test.js:155:25
  at usingTempDir (integration/create-next-app/index.test.js:23:9)
  at Object.<anonymous> (integration/create-next-app/index.test.js:153:9)

● create next app › should use --example-path over the file path in the GitHub URL

Command failed with exit code 1 (EPERM): node /home/runner/work/next.js/next.js/packages/create-next-app/dist/index.js github-example-path-2 --example https://github.com/vercel/next-learn-starter/tree/master/navigate-between-pages-starter --example-path navigate-between-pages-starter

  172 |       expect(
  173 |         fs.existsSync(path.join(cwd, projectName, '.gitignore'))
> 174 |       ).toBeTruthy()
      |                     ^
  175 |       expect(
  176 |         fs.existsSync(path.join(cwd, projectName, 'node_modules/next'))
  177 |       ).toBe(true)

  at makeError (../node_modules/execa/lib/error.js:58:11)
  at handlePromise (../node_modules/execa/index.js:112:26)
      at runMicrotasks (<anonymous>)
  at integration/create-next-app/index.test.js:174:25
  at usingTempDir (integration/create-next-app/index.test.js:23:9)
  at Object.<anonymous> (integration/create-next-app/index.test.js:172:9)

● create next app › should use npm as the package manager on supplying --use-npm with example

Command failed with exit code 1 (EPERM): node /home/runner/work/next.js/next.js/packages/create-next-app/dist/index.js use-npm --use-npm --example https://github.com/vercel/next-learn-starter/tree/master/learn-starter

  326 |         '.eslintrc.json',
  327 |       ]
> 328 |       files.forEach((file) =>
      |                         ^
  329 |         expect(fs.existsSync(path.join(cwd, file))).toBeTruthy()
  330 |       )
  331 |     })

  at makeError (../node_modules/execa/lib/error.js:58:11)
  at handlePromise (../node_modules/execa/index.js:112:26)
  at integration/create-next-app/index.test.js:328:25
  at usingTempDir (integration/create-next-app/index.test.js:23:9)
  at Object.<anonymous> (integration/create-next-app/index.test.js:326:9)

docs/authentication.md Outdated Show resolved Hide resolved
Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

Looking great!

Co-authored-by: Lee Robinson <me@leerob.io>
@ijjk ijjk merged commit 713a5aa into vercel:canary Sep 17, 2021
@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue was opened via the examples template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants