feat(client): improved basepath detection #2320

mahieyin-rahmun · 2021-07-06T11:30:01Z

Reasoning 💡

This PR attempts to improve client side path detection by leveraging a second environment variable named NEXT_PUBLIC_NEXTAUTH_URL to ensure client side path detections are always consistent, regardless of folder structure and URL.

Currently, there are some issues with the client side path detection. e.g. signIn() function assumes that the folder structure is like the following:

test-project
    └── pages
        └── api
            └── auth
                └── [...nextauth].js

and NEXTAUTH_URL to be set to http://localhost:3000 or https://mydomain.com. The above breaks if the URL and/or folder structure of the user's project is different.

With these changes, users will be able to use a different folder structure if they want, such as:

test-project
└── pages
    └── api
       └── v1
          └── authentication
             └── [...nextauth].js

And set NEXTAUTH_URL to something like http://localhost:3000/api/v1/authentication. As compensation for the flexibility, they now need to declare a second environment variable:

NEXTAUTH_URL=http://localhost:3000/api/v1/authentication
NEXT_PUBLIC_NEXTAUTH_URL=http://localhost:3000/api/v1/authentication

Luckily, this can be simplified by using variables expansion feature of Next.js

NEXTAUTH_URL=http://localhost:3000/api/authentication
NEXT_PUBLIC_NEXTAUTH_URL=$NEXTAUTH_URL

Checklist 🧢

Documentation
Tests
Ready to be merged

Affected issues 🎟

Closes #1713, #2316. Related to #689, #1712, #1517, #900, #499, #1676

vercel · 2021-07-06T11:30:07Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextauthjs/next-auth/24v71m2c6rkNcs5xN5bYyb3Ttwih
✅ Preview: https://next-auth-git-fork-mahieyin-rahmun-feat-impro-780de8-nextauthjs.vercel.app

codecov-commenter · 2021-07-06T11:31:22Z

Codecov Report

Merging #2320 (4d77a53) into next (a2e5afa) will increase coverage by 0.11%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             next    #2320      +/-   ##
==========================================
+ Coverage   10.74%   10.86%   +0.11%     
==========================================
  Files          83       83              
  Lines        1414     1427      +13     
  Branches      393      402       +9     
==========================================
+ Hits          152      155       +3     
- Misses       1042     1050       +8     
- Partials      220      222       +2

Impacted Files	Coverage Δ
src/client/react.js	`81.70% <0.00%> (-2.57%)`	⬇️
src/server/index.js	`0.00% <0.00%> (ø)`
src/lib/parse-url.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2e5afa...4d77a53. Read the comment docs.

ubbe-xyz

Awesome 💯 ✨ , I think we should think this twice, whether we really want to introduce another environment variable on the setup or not 👍🏽

I left some cleanup suggestions as well 💬

ubbe-xyz · 2021-07-06T12:36:45Z

src/client/__tests__/parse-url.test.js

+    jest.resetModules() // Most important - it clears the cache
+    process.env = { ...OLD_ENV } // Make a copy
+  })
+
+  afterAll(() => {
+    process.env = OLD_ENV // Restore old environment


I think there's no need for comments here, the code is pretty self-descriptive 👍🏽

Makes sense.

ubbe-xyz · 2021-07-06T12:53:56Z

src/lib/parse-url.js


+  if (!url) {
+    if (process.env.NEXT_PUBLIC_NEXTAUTH_URL) {


I have the impression we should move this to v4 and change NEXTAUTH_URL to be NEXT_PUBLIC_NEXTAUTH_URL. In this way, we can keep a single environment variable and prevent code complexity by having to manage two of them. What do you think @balazsorban44 ? 🧶

Yes, this is intended to be part of v4, otherwise it would be a big breaking change, IMO. I made the pull request against the next branch.

I like the idea of having a single variable.

ubbe-xyz · 2021-07-06T13:03:07Z

src/lib/parse-url.js

@@ -5,23 +5,36 @@
 * supporting a default value, so a simple split is sufficent.
 * @param {string} url
 */
-export default function parseUrl (url) {
+export default function parseUrl(url) {


This function is also called server-side, and the logic currently works because there we pass it NEXTAUTH_URL.

I think we should move that logic inside, hence whenever calling parseUrl it can automatically compute what the right base URL and base path are:

export default function parseBaseUrl(url) { // .. if (process.env.NEXTAUTH_URL) { if (!process.env.NEXT_PUBLIC_NEXTAUTH_URL) { throw new Error("[next-auth] Trying to use a custom `NEXTAUTH_URL` without setting `NEXT_PUBLIC_NEXTAUTH_URL`, client-side redirects won't work")

Now that we're modifying this function, I think we should:

rename it as it's not generic utility about URLs but is intended to compute the base URL

throw in case expectations don't match so we're explicit about mis-configurations

I think we should move that logic inside, hence whenever calling parseUrl it can automatically compute what the right base URL and base path are

Yes, currently, it feels like it's dangling between both client and server.

throw in case expectations don't match so we're explicit about mis-configurations

Agreed.

ubbe-xyz · 2021-07-06T13:05:12Z

src/server/index.js

+if (
+  process.env.NODE_ENV === "development" &&
+  !/^http:\/\/localhost:\d+$/.test(process.env.NEXTAUTH_URL) &&
+  !process.env.NEXT_PUBLIC_NEXTAUTH_URL
+) {
+  logger.warn(
+    "NEXT_PUBLIC_NEXTAUTH_URL",
+    `NEXTAUTH_URL is set to "${process.env.NEXTAUTH_URL}" instead of the default "http://localhost:\${PORT}", and NEXT_PUBLIC_NEXTAUTH_URL is not set. Client side path detections will fail.`
+  )
+}


In my opinion, we should move this logic to parseUrl and the check above too to start sliming down this monolith file.

On the other hand, we should be throwing errors in both cases rather than warnings as this misconfiguration, I understand, is critical? 🤔

On the other hand, we should be throwing errors in both cases rather than warnings as this misconfiguration, I understand, is critical?

I agree on this. I could not think of a better message, so I left it as warning, but it should definitely be treated as error.

www/docs/getting-started/rest-api.md

ubbe-xyz · 2021-07-06T13:11:39Z

src/client/__tests__/parse-url.test.js

+    url = "http://localhost:3000/api/v1/authentication/"
+    // this semi-colon is needed, otherwise js thinks the string
+    // above is a function
+    ;({ baseUrl, basePath } = parseUrl(url))


As I understand we don't need to re-assign url here, why not just do:

const url1 = "http://localhost:3000" const { baseUrl: baseUrl1, basePath: basePath1 } = parseUrl(url1) // ... const url2 = "http://localhost:3000" const { baseUrl: baseUrl2, basePath: basePath2 } = parseUrl(url2)

I think will make this case a bit nicer to read 👍🏽

I wanted to do this but decided against it. But it seems like it would actually be better this way.

ubbe-xyz · 2021-07-06T13:12:11Z

src/client/__tests__/parse-url.test.js

+
+    expect(baseUrl).toBe("https://www.mydomain.com")
+    expect(basePath).toBe("/api/v3/auth")
+  })


We probably want to add test cases for the unhappy paths as well 🙏🏽

Yup. That would probably make more sense once we have refactored parseUrl() to throw errors.

ubbe-xyz · 2021-07-06T13:14:43Z

src/client/react.js

@@ -56,7 +56,7 @@ export function useSession(options = {}) {

  React.useEffect(() => {
    if (requiredAndNotLoading) {
-      const url = `/api/auth/signin?${new URLSearchParams({
+      const url = `${_apiBaseUrl()}/signin?${new URLSearchParams({


I think we could use parseUrl() rather than having duplicate utils?

I am not sure on this one.

As far as I understand, _apiBaseUrl() checks the __NEXTAUTH object, which in turn makes use of the parseUrl() function. So technically it could be refactored.

I would leave it on the core team to make decisions on this and refactor accordingly.

Good point, I didn't realise _apiBaseUrl() calls parseUrl() internally, as you mention worth of a refactor in a different PR 👍🏽

@mahieyin-rahmun could you move this change to a different PR please? 🙏🏽

updated as per review request Co-authored-by: Lluis Agusti <[email protected]>

Naddiseo · 2021-07-06T14:36:06Z

src/lib/parse-url.js

+  const protocol = url.startsWith("http:") ? "http" : "https"

  // Normalize URLs by stripping protocol and no trailing slash
-  url = url.replace(/^https?:\/\//, '').replace(/\/$/, '')
+  url = url.replace(/^https?:\/\//, "").replace(/\/$/, "")

  // Simple split based on first /
-  const [_host, ..._path] = url.split('/')
+  const [_host, ..._path] = url.split("/")
  const baseUrl = _host ? `${protocol}://${_host}` : defaultHost
-  const basePath = _path.length > 0 ? `/${_path.join('/')}` : defaultPath
+  const basePath = _path.length > 0 ? `/${_path.join("/")}` : defaultPath


Out of curiosity, is this something that could be accomplished using URL ?

Something like:

let baseUrl = defaultHost; let basePath = defaultPath; try { url = new URL(url, url.startsWith('http') ? undefined : defaultHost); baseUrl = url.origin; basePath = url.pathname.length ? url.pathname : defaultPath; } catch (e) {}

Interesting PoV. I will try and test this out. Looks like this could work, with some modifications.

balazsorban44 · 2021-07-10T11:15:18Z

I haven't had a closer look yet, but I would definitely keep a single env variable, not introduce a new one. adding NEXT_PUBLIC_NEXTAUTH_URL and removing NEXTAUTH_URL seems reasonable, but it feels like such a drastic change since it will require changing people's pipelines etc. I know this will be a major release, but still.

Another way of inlining (making it available client-side) an env variable is to use the env option in the next.config.js file ref. This way changing the variable name wouldn't be necessary, we would only need to inform users about this. Although I am not sure if this is something Next.js will keep around.

I think only a very few of our users will actually need this change at all. Most of the time /api/auth is sufficient to be assumed.

I can be convinced otherwise, but I don't think we should keep both variables either way.

I have some ideas, maybe we could even eliminate the need for such a variable at all.. The whole reason we keep it around is because of our "isomorphic" functions like getSession, which are supposed to work both client and server-side.

The client actually works without full URLs, it's the server-side calls that need them. But this is a suboptimal solution already, as pointed out in #1535. We should maybe just get rid of the thought of "isomorphic" and have fully separate methods for backend and front-end.

Forget it. We have more logic in the server-side that relies on this variable. (Eg.: redirects)

mahieyin-rahmun · 2021-07-10T14:11:20Z

I haven't had a closer look yet, but I would definitely keep a single env variable, not introduce a new one. adding NEXT_PUBLIC_NEXTAUTH_URL and removing NEXTAUTH_URL seems reasonable, but it feels like such a drastic change since it will require changing people's pipelines etc. I know this will be a major release, but still.

I agree about the one variable part, and I understand about it being a big change, but I would argue that with the amount of breaking changes in v4 already, people would have to shift around bits of their codebase anyway. An environment variable, IMHO, should not be that big of a deal. Those who don't want to, can stick with v3, which seems like a reasonable tradeoff. And most new users would start off with v4 anyway.

Another way of inlining (making it available client-side) an env variable is to use the env option in the next.config.js file ref. This way changing the variable name wouldn't be necessary, we would only need to inform users about this. Although I am not sure if this is something Next.js will keep around.

Yes, they introduced NEXT_PUBLIC_ prefix in lieu of this, and they recently did away with webpack v4 in favor of v5, so I don't think it will stay in the long run either.

I think only a very few of our users will actually need this change at all. Most of the time /api/auth is sufficient to be assumed.

IMHO, there are quite a few issues that talk about this, and although it probably is a small portion of the userbase, it would be a good addition, since it keeps everything dynamic as opposed to hardcoded or working on assumptions.

ubbe-xyz · 2021-07-11T18:08:45Z

@mahieyin-rahmun I've been speaking about this with @balazsorban44, we think adding NEXT_PUBLIC_NEXTAUTH_URL v4.0.0 would be too disruptive, as consumers could easily bypass the update and end in quite a few broken pipelines 😅

We think it's something we could add as a feature in a later v4 release, like 4.x.0 for example, and keep both environment variables around until v5 and that would give enough time to people to migrate, and time for us to educate about it.

Meanwhile, you can use one of the two solutions mentioned here:
#689 (comment)

use env field of next.config.js
supply it as a basePath to <Provider />

that would get you going 👍🏽

ubbe-xyz · 2021-07-11T18:12:55Z

@mahieyin-rahmun, we're gonna close this 👍🏽 , but we keep an eye on it 👀 for when we add this back on 4.x.0 🙏🏽 , super thanks for the PR 💚 💯

mahieyin-rahmun · 2021-07-13T06:33:16Z

@mahieyin-rahmun, we're gonna close this 👍🏽 , but we keep an eye on it for when we add this back on 4.x.0 🙏🏽 , super thanks for the PR

@lluia, Sure, let me know when it is time, I am willing to open another PR with the suggested refactors/modifications in this PR.

mahieyin-rahmun added 3 commits July 6, 2021 17:15

feat(client): improve client side path detections

f285a33

feat(client): add tests for improved client side path detection

5ec8da5

docs(client): update docs

e2c3c25

vercel bot temporarily deployed to Preview July 6, 2021 11:30 Inactive

github-actions bot added client Client related code core Refers to `@auth/core` docs Relates to documentation labels Jul 6, 2021

docs(client): fix typo

e53e31b

vercel bot temporarily deployed to Preview July 6, 2021 11:36 Inactive

ubbe-xyz requested changes Jul 6, 2021

View reviewed changes

Update www/docs/getting-started/rest-api.md

4d77a53

updated as per review request Co-authored-by: Lluis Agusti <[email protected]>

vercel bot temporarily deployed to Preview July 6, 2021 13:31 Inactive

Naddiseo reviewed Jul 6, 2021

View reviewed changes

balazsorban44 self-requested a review July 6, 2021 19:47

balazsorban44 self-assigned this Jul 6, 2021

ubbe-xyz closed this Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): improved basepath detection #2320

feat(client): improved basepath detection #2320

mahieyin-rahmun commented Jul 6, 2021

vercel bot commented Jul 6, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2021 •

edited

Loading

ubbe-xyz left a comment

ubbe-xyz Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 6, 2021 •

edited

Loading

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

ubbe-xyz Jul 8, 2021

ubbe-xyz Jul 11, 2021

Naddiseo Jul 6, 2021

mahieyin-rahmun Jul 6, 2021

balazsorban44 commented Jul 10, 2021 •

edited

Loading

mahieyin-rahmun commented Jul 10, 2021

ubbe-xyz commented Jul 11, 2021

ubbe-xyz commented Jul 11, 2021

mahieyin-rahmun commented Jul 13, 2021

feat(client): improved basepath detection #2320

feat(client): improved basepath detection #2320

Conversation

mahieyin-rahmun commented Jul 6, 2021

Reasoning 💡

Checklist 🧢

Affected issues 🎟

vercel bot commented Jul 6, 2021 • edited Loading

codecov-commenter commented Jul 6, 2021 • edited Loading

Codecov Report

ubbe-xyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ubbe-xyz Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balazsorban44 commented Jul 10, 2021 • edited Loading

mahieyin-rahmun commented Jul 10, 2021

ubbe-xyz commented Jul 11, 2021

ubbe-xyz commented Jul 11, 2021

mahieyin-rahmun commented Jul 13, 2021

vercel bot commented Jul 6, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2021 •

edited

Loading

ubbe-xyz Jul 6, 2021 •

edited

Loading

balazsorban44 commented Jul 10, 2021 •

edited

Loading