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

Improve client-side basePath detection #1713

Closed
balazsorban44 opened this issue Apr 13, 2021 · 9 comments
Closed

Improve client-side basePath detection #1713

balazsorban44 opened this issue Apr 13, 2021 · 9 comments
Labels
enhancement New feature or request stale Did not receive any activity for 60 days

Comments

@balazsorban44
Copy link
Member

balazsorban44 commented Apr 13, 2021

Summary of proposed feature
Currently, when the NEXTAUTH_URL env variable contains a basePath also defined in next.config.js, the next-auth/client still cannot pick it up. This is because when Next.js builds the production bundle, it will ignore all environment variables in client code, that is not prepended with NEXT_PUBLIC_ and inline them otherwise. This is described in their documentation here: https://nextjs.org/docs/basic-features/environment-variables#exposing-environment-variables-to-the-browser

Purpose of proposed feature

Iain's suggestion - where you pass basePath to the options prop of Provider - might work, but you will have to duplicate your basePath, so we might just want to make sure that we also read in NEXT_PUBLIC_NEXTAUTH_URL instead.

Detail about proposed feature

This will ensure that all the URLs have a single source of truth and that the actual path will be inlined at build-time.

Potential problems

Yet another variable might be confusing to users, but I don't see a better solution immediately. Open for suggestions.

Describe any alternatives you've considered

A solution is described here: #689 (comment), but it requires duplication. (Eg.: You already implicitly set basePath through NEXTAUTH_URL, it shouldn't be necessary to duplicate this, especially when we provide an isomorphic next-auth/client module.)

Additional context

Related: #689, #1712, #1517, #900, #499, #1676

Please indicate if you are willing and able to help implement the proposed feature.
I will wait for this to get feedback or if anyone has a better idea, please comment below!

@balazsorban44 balazsorban44 added the enhancement New feature or request label Apr 13, 2021
@balazsorban44 balazsorban44 changed the title Improve client-side basePath support Improve client-side basePath detection Apr 17, 2021
@lavcraft
Copy link

lavcraft commented Apr 18, 2021

Hi. NEXT_PUBLIC_NEXTAUTH_URL is good solution, but i expect next problems:

  1. According to docs, NEXT_PUBLIC_NEXTAUTH_URL available only when you build your app with NEXT_PUBLIC_NEXTAUTH_URL env var. Instead - it's not available
  2. If you want to access to vars, you should add getInitialProps or getServerSideProps - and it's not transparent condition and add new responsibility for developer.
    For example - NEXT_PUBLIC_ env vars not expose vercel/next.js#23734. In this issue @nlaitan add getInitialProps to _app. But it prevent static optimization for all pages :( In my opinion it's not good solution. But any good solution required a standard for reliable and transparent way to pass ENV vars to browser/server configuration

@balazsorban44
Copy link
Member Author

balazsorban44 commented Apr 18, 2021

The value will be INLINED at build time meaning that the reference to process.env.NEXT_PUBLIC_NEXTAUTH_URL would be swapped with a hard-coded string when you run next build. You don't need any configuration through getInitialProps or getServerSideProps for this. 😉

You won't expect the NEXTAUTH_URL to change in the lifetime of your deploy anyway.

Update:

I can see you got the same answer from one of the Next.js team member here: vercel/next.js#23734 (comment)

@lavcraft
Copy link

@balazsorban44 right, but for me it's not reliable tool. I want to build once and run anywhere :)
For example - i have many CDN urls and regions. Building app for each region with custom configuration - mindblowing task :)

Ability to delegate this to infrastructure is a good DevOps practice, because we get one tested artifact and run in many different environments and test only particular cases on specific region.

@lavcraft
Copy link

lavcraft commented Apr 18, 2021

And see how many people operate with this (small text in docs is hard to understand :))
For example - vercel/next.js#23734 (comment)
Many people add getInitialProps to _app as Golden solution for this problem :) I guess it's not good user experience.

@balazsorban44 thank you for your advices, i'm really appreciate :) I just want to improve this use cases, because believe - it can be more friendly and cover more cases. But in the same time i understand - many people use next.js in other cases, like static only sites. I'm using in combined variant - static + ssr. It's not widely used combination i guess :) In this place we have trade-off between easy to implement build time vars, and reliable configuration system, which cover all cases :)

@balazsorban44
Copy link
Member Author

balazsorban44 commented Apr 18, 2021

static + ssr is a very valid case, that's the whole point of Next.js. So I don't totally disagree, but the thing is, many OAuth providers are very rigid about their callback urls, some of them only accept a single one, and probably none accepts wildcards or similar. So configuration won't be automated even if we could provide a way of dynamically setting this value, you will still have to manually update the providers you use.

Env variables cover MOST of our users' usecases, and is relatively easy to use. I think that is why @iaincollins turned away from a dynamic option for setting this url. (which if I remember correctly was the case for v2)

But I think we are starting to deviate from the original topic here. This issue only concerns a way of setting the basePath automatically to match the one set in the backend.

Whether there is a better way than env variables is a topic for another day. I cannot dig it up now, but Iain had a good explanation why this is the solution he went with in v3. (Basically it was the least worst option)

@CodaBool
Copy link

@balazsorban44

I cannot dig it up now, but Iain had a good explanation why this is the solution

He mentions that in #674

I'm coming at this issue from the new @sls-next/serverless-component (2k stars | 8k downloads / week)
and this issue is very relevant for anyone who is using it and next-auth.
The reason for that is the framework is built to run on Lambda@Edge which currently does not support environment variables

lambda@edge limitations

For now I am using a workaround of setting baseUrl as an option in the Provider. This works for me since I am only using the credentials provider for right now and callbacks are predictable. When I make use of more providers and may run into the callback issues mentioned I may come back to assist on this issue.

@stale stale bot added the stale Did not receive any activity for 60 days label Jun 20, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Jun 20, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Jun 20, 2021
@balazsorban44
Copy link
Member Author

balazsorban44 commented Aug 7, 2021

I created a reproduction and suggestions for a solution here (mostly for internal discussion, but might useful for others):

https://github.com/balazsorban44/next-auth-base-path

look for .env, next.config.js and the _app file, and notice the path for the api handler is /api/custom/[...nextauth.js

@stale stale bot added the stale Did not receive any activity for 60 days label Oct 7, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Oct 7, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Oct 7, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Dec 6, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Dec 6, 2021
@stale stale bot removed the stale Did not receive any activity for 60 days label Dec 6, 2021
@stale
Copy link

stale bot commented Feb 5, 2022

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Feb 5, 2022
@stale
Copy link

stale bot commented Feb 12, 2022

Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale Did not receive any activity for 60 days
Projects
None yet
Development

No branches or pull requests

3 participants