Nextjs-auth0: The code quality of the examples is low

Created on 18 May 2020  Â·  7Comments  Â·  Source: auth0/nextjs-auth0

Describe the problem you'd like to have solved

There's IMO a number of flaws in the examples:

  • It's possible to accidentally use the global user from 'fetchUser' so that you log in others inadvertently (API2)
  • You're printing internal errors on failure
  • You use the error handler to set the status code, when IRL the status code might already have been set by the auth0 lib; causing an exception that crashes the server because headers already have been sent
  • The components' files are not properly capitalised (e.g. Layout.jsx) and it would be nice, had they been TypeScript, and it ought to be withAuth.jsx not with-auth.jsx (niggles only)
  • E.g. login-redirect.jsx only redirects client-side, not server-side, potentially breaking the back button
  • In e.g. with-auth.jsx you do a if (!ctx.req) { instead of the more common if (typeof window !== 'undefined') { — because this is basically just a check if we're running in the browser, so it should clearly be written as such
  • In with-auth you overwrite any/all getInitialProp values that pages might have
  • It's not a well written HoC; instead of returning div > InnerComponent, it should return a wrapper around what it received — and Layout should absolutely not require you to pass two generic properties loading and user
  • You should split up user.js into its different parts
  • NextJS now includes a fetch global, so you can remove the isomorphic-unfetch import
  • Never compare to undefined in JS, since it's a valid variable name; so userState === undefined is buggy and should be typeof userState === 'undefined'
  • The exported UserProvider is in fact NOT a user provider; it's some sort of wrapper around it
  • You have extra isMounted checks when you don't need to
  • There's no way to force a user refresh; once set globally in the app, it's there forever
  • The pattern with import config from './config' is just strange and completely needless since the indirection doesn't add expressive power

Describe the ideal solution

Fixing all the above. I've done it for now, I can file a PR; but that includes TypeScript typings for everything; looks like this:

image

Alternatives and current work-arounds

There are some not-so-nice bugs in there.

Question

Would you accept a PR that fixes this but turns it into TypeScript?

Most helpful comment

I would also like to add, that by adding all environment variables to the next.config.js they will get included in the client bundle, potentially exposing critical information.

Environment variables only needed server-side do not (and should not) need to be included in next.config.js

All 7 comments

I would also like to add, that by adding all environment variables to the next.config.js they will get included in the client bundle, potentially exposing critical information.

Environment variables only needed server-side do not (and should not) need to be included in next.config.js

Additional point: Due to restrictions on the number of lambdas when next is hosted on Vercel

The four auth0 apis could be collapsed into one relative path file such as: api/auth/[...path].tsx, with path switching built into the one api file. Turning 4 lambdas into 1, which might help smaller projects keep under the limit on lambdas

@BjoernRave Are you sure? https://nextjs.org/docs/api-reference/next.config.js/introduction says:

next.config.js is a regular Node.js module, not a JSON file. It gets used by the Next.js server and build phases, and it's not included in the browser build.

Thie sample also seems to use next.config.js.

I believe in Next.js 9.4 the need to declare environment variables in next.config.js is removed (at least for browser accessible environment variables):
https://nextjs.org/blog/next-9-4#new-environment-variables-support

If you read the section in the blog of 9.4. it at least seems like having the env vars in next.config.js exposes them in the browser bundle: https://nextjs.org/blog/next-9-4#new-environment-variables-support

First, you can now prefix the environment variable with NEXT_PUBLIC_ to expose an environment variable to the browser. When that environment variable is used it will then be inlined into the browser JavaScript bundle.

You no longer have to add a next.config.js and add the env key to expose these variables.

@haf , it would be awesome if you could file a PR, im quite new to javascript world and learning by reading examples, would love to see your fixups :)

Hi @haf, please check out the updated samples in the new v1.0.0-beta.0 release.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

RyanPayso13 picture RyanPayso13  Â·  4Comments

synoptase picture synoptase  Â·  6Comments

jvorcak picture jvorcak  Â·  4Comments

aorsten picture aorsten  Â·  6Comments

lkbr picture lkbr  Â·  4Comments