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: Option to disable inlining of public env vars for node server #41396

Closed
wants to merge 1 commit into from

Conversation

revmischa
Copy link

What

Add an experimental option to disable the inlining of NEXT_PUBLIC_ env vars for the node server.

Why

I am maintaining on a CDK construct to deploy NextJS to AWS using the standalone output mode. The issue that I have is that I want to allow env vars to be provided by AWS infrastructure, things like API URLs that aren't resolved until after your deployment completes. This means that certain env vars are not available at Next build time.

I can work around this by setting environment variables for the server and rewriting the static values after deployment (this is much trickier with the server code for esoteric reasons), but I need the standalone server JS bundles to read the actual environment variables and not rewrite them at build-time.

Fixes #40897, discussion here: #40482

Bug

  • Related issues linked using fixes #number
  • Integration tests added
  • Errors have a helpful link attached, see contributing.md

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.
  • Errors have a helpful link attached, see contributing.md

Documentation / Examples

  • Make sure the linting passes by running pnpm lint
  • The "examples guidelines" are followed from our contributing doc

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, can you explain why non NEXT_PUBLIC_ env variables can't be used instead? It sounds like NEXT_PUBLIC_ is being used incorrectly here causing the issue.

@revmischa
Copy link
Author

revmischa commented Oct 13, 2022

Hi, can you explain why non NEXT_PUBLIC_ env variables can't be used instead? It sounds like NEXT_PUBLIC_ is being used incorrectly here causing the issue.

Say I want my frontend to talk to my AWS Cognito user pool. I need to provide the both the frontend and backend with the user pool ID. So I create NEXT_PUBLIC_COGNITO_USER_POOL_ID and read it in both places.

@revmischa
Copy link
Author

Furthermore, this is a generic solution for anyone running NextJS. I don't have full control of how users will use their public env vars. They may do things "incorrectly" but have a really unpleasant time tracking down this problem. I would like this to be more dummy-proof.

@ijjk
Copy link
Member

ijjk commented Oct 13, 2022

If it's inlined for the client but not the backend it seems like it could cause accidental mis-matching between the two which seems unideal.

@revmischa
Copy link
Author

If it's inlined for the client but not the backend it seems like it could cause accidental mis-matching between the two which seems unideal.

The values will be resolved to the same thing. They both come from process.env.

@ijjk
Copy link
Member

ijjk commented Oct 13, 2022

If one is build time (client inlining) and the other is dynamic (server skipped inlining) the dynamic value could be accidentally changed without updating the inlined client value causing the mismatch right?

@revmischa
Copy link
Author

If one is build time (client inlining) and the other is dynamic (server skipped inlining) the dynamic value could be accidentally changed without updating the inlined client value causing the mismatch right?

Yes true, in theory I suppose that would be possible but it's all pretty automated and deterministic with CDK. No one is manually editing the env vars of the server lambda function; they're all populated in each deployment, which is atomic.

@ijjk
Copy link
Member

ijjk commented Oct 13, 2022

I'm not sure I follow, if they are never edited then what's the problem with them being inlined?

I want to allow env vars to be provided by AWS infrastructure, things like API URLs that aren't resolved until after your deployment completes. This means that certain env vars are not available at Next build time

If they aren't available during build it doesn't seem to make sense to call name them NEXT_PUBLIC_ since they would then be runtime env vars instead which should work fine for the use case described.

@revmischa
Copy link
Author

I'm not sure I follow, if they are never edited then what's the problem with them being inlined?

These are resolved cloudformation values. Like the name of a S3 bucket or a URL or function ARN. The problem with them being inlined is that the values are not known at build-time, they are known after the cloudformation stack is deployed. It's possible to set placeholder values at build-time and replace them after the app is deployed. This is trivial to do for frontend JS because it gets deployed to an S3 bucket and is normal files.
It's much more complicated to do with lambda function code which is not made up of S3 objects and requires symlinks to be preserved for pnpm node modules in standalone output mode. It's much more straightforward to provide those as environment variables in the lambda function.

If they aren't available during build it doesn't seem to make sense to call name them NEXT_PUBLIC_ since they would then be runtime env vars instead which should work fine for the use case described.

I think it makes sense, and others may think it makes sense. I am not building a NextJS app, I'm building something to deploy other peoples' NextJS apps, so I am trying to make something that will have the least surprising behavior.

@revmischa
Copy link
Author

Please 🙏🏻
This will make the CDK deployment so much simpler

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Going to close this as stale, feel free to open a fresh PR if still relevant and we can look at adding this experimental config for testing!

@ijjk ijjk closed this Jun 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants