inline project env files in worker #181

james-elicx · 2024-12-15T23:18:15Z

Changes

Extracts env vars from .env files.
Chucks them into a .env.mjs that is dynamically imported in the worker.

changeset-bot · 2024-12-15T23:18:18Z

⚠️ No Changeset found

Latest commit: a5268a7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

pkg-pr-new · 2024-12-15T23:18:54Z

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@181

commit: dbb57bf

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts

packages/cloudflare/src/cli/templates/worker.ts

vicb · 2024-12-16T09:25:46Z

A few thoughts (I'll do a deeper review later today)

I find "build time" to be confusing. Looking at the next docs there are 2 things: Non-NEXT_PUBLIC_ and NEXT_PUBLIC_ vars.

Does the current code support referencing other vars - maybe add a test.

It looks like we should support development, production and test. It looks like get the env from NODE_ENV so maybe applyBuildTimeEnv(env["NEXTJS_ENV"]) should actually be applyBuildTimeEnv(env["NEXTJS_ENV"] ?? process.env.NODE_ENV)

To be more consistent with how Open Next work, we might want to generate .env.development.mjs, ... in the build folder of open next and include the file from worker.ts (rather than ESBulding the file). That would be similar to compiling the config

packages/cloudflare/src/cli/templates/worker.ts

james-elicx · 2024-12-17T09:28:30Z

I find "build time" to be confusing. Looking at the next docs there are 2 things: Non-NEXT_PUBLIC_ and NEXT_PUBLIC_ vars.

Removed the reference to 'build-time'.

Does the current code support referencing other vars - maybe add a test.

Yeah, that was actually why i used the newer dotenvx instead of dotenv :). Added a test.

It looks like we should support development, production and test. It looks like get the env from NODE_ENV so maybe applyBuildTimeEnv(env["NEXTJS_ENV"]) should actually be applyBuildTimeEnv(env["NEXTJS_ENV"] ?? process.env.NODE_ENV)

To be more consistent with how Open Next work, we might want to generate .env.development.mjs, ... in the build folder of open next and include the file from worker.ts (rather than ESBulding the file). That would be similar to compiling the config

Sorry I hadn't realised it was an intentional part of the design. I've undone that change. Instead, it's not putting all of them into an .env.mjs file, exporting each mode. I originally did .env.${mode}.mjs, but this was causing runtime issues with variable-based dynamic imports due to wrangler's bundle step, so i couldn't do that unfortunately.

packages/cloudflare/src/cli/build/patches/investigated/copy-package-cli-files.ts

packages/cloudflare/src/cli/build/patches/investigated/copy-env-files.ts

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts

vicb · 2024-12-18T12:24:14Z

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts

+ * In a monorepo, the env files in an app's directory will take precedence over
+ * the env files at the root of the monorepo.
+ */
+export function extractProjectEnvVars(mode: string, options: BuildOptions) {


Doesn't that mean that we will always use the .local files while we only want to use them for wrangler dev but not in prod?

Yeah. How come you wouldn't want them in production? Next.js would use those files. I suppose you're talking about if someone builds it locally and then deploys, but in that case, you could make the argument that we shouldnt include any .env files at all because technically they're all local to the machine. I'm not quite sure what else you could do here - open to suggestions though.

@james-elicx I did a quick test and you are right, .local files are used if present, regardless if it is a prod or dev build. I was wrong to think that they would only be used in dev.

packages/cloudflare/src/cli/templates/worker.ts

…ckage-cli-files.ts

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts

packages/cloudflare/src/cli/build/patches/investigated/copy-env-files.ts

vicb · 2024-12-18T17:52:30Z

Do you think you can add a simple test, maybe to the api example app?

packages/cloudflare/src/cli/build/bundle-server.ts

packages/cloudflare/src/cli/build/patches/investigated/compile-env-files.ts

vicb

Awesome PR, thanks!

I would move the code out of patch*, I think it should be part of the build.

Approving the PR, you can pick whatever your prefer.

🙏

* extract env vars from file system * combine variables from a global with the request-scoped env * inline build-time env vars in the worker script * for some reason the tests failed in the pipeline but not locally * switch between modes at runtime and apply on process.env * add test for referencing variables * use a .env.mjs file for the vars * Update packages/cloudflare/src/cli/build/patches/investigated/copy-package-cli-files.ts * move the merging to extractProjectEnvVars * rename secrets to nextEnvVars * add missing mode when retrieving value * add link to nextjs var load order * rename to compile * change function to read a single file * move the readEnvFile call inside the flatMap * remove process.env.node_env usage * add e2e test for env vars * move locations

james-elicx marked this pull request as ready for review December 15, 2024 23:22

james-elicx requested review from dario-piotrowicz, petebacondarwin and vicb December 15, 2024 23:22

james-elicx linked an issue Dec 15, 2024 that may be closed by this pull request

Next .env file #157

Closed

vicb reviewed Dec 16, 2024

View reviewed changes

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts Outdated Show resolved Hide resolved

packages/cloudflare/src/cli/templates/worker.ts Outdated Show resolved Hide resolved

james-elicx force-pushed the james/inline-env branch from b8c29e4 to 782f0d1 Compare December 16, 2024 08:20

james-elicx requested a review from vicb December 16, 2024 08:22

james-elicx force-pushed the james/inline-env branch from 782f0d1 to a0ca689 Compare December 17, 2024 09:25

james-elicx commented Dec 17, 2024

View reviewed changes

packages/cloudflare/src/cli/templates/worker.ts Outdated Show resolved Hide resolved

james-elicx commented Dec 17, 2024

View reviewed changes

packages/cloudflare/src/cli/build/patches/investigated/copy-package-cli-files.ts Outdated Show resolved Hide resolved

james-elicx changed the title ~~inline build-time env files in worker~~ inline project env files in worker Dec 17, 2024

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/patches/investigated/copy-env-files.ts Show resolved Hide resolved

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts Outdated Show resolved Hide resolved

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/templates/worker.ts Outdated Show resolved Hide resolved

james-elicx added 10 commits December 18, 2024 13:25

extract env vars from file system

181910c

combine variables from a global with the request-scoped env

fcd1156

inline build-time env vars in the worker script

c8ac7d8

for some reason the tests failed in the pipeline but not locally

3ffdc83

switch between modes at runtime and apply on process.env

df61c41

add test for referencing variables

d3163b3

use a .env.mjs file for the vars

ae2b8ee

Update packages/cloudflare/src/cli/build/patches/investigated/copy-pa…

9ff653c

…ckage-cli-files.ts

move the merging to extractProjectEnvVars

e740d18

rename secrets to nextEnvVars

ea4bb69

add missing mode when retrieving value

c43a925

james-elicx force-pushed the james/inline-env branch from 8beee04 to c43a925 Compare December 18, 2024 13:27

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts Show resolved Hide resolved

james-elicx commented Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/utils/extract-project-env-vars.ts Show resolved Hide resolved

add link to nextjs var load order

64e51a5

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/patches/investigated/copy-env-files.ts Outdated Show resolved Hide resolved

james-elicx added 3 commits December 18, 2024 16:36

rename to compile

8e5b382

change function to read a single file

bf62788

move the readEnvFile call inside the flatMap

a107abb

james-elicx added 2 commits December 18, 2024 18:44

remove process.env.node_env usage

a9f7ec4

add e2e test for env vars

a5268a7

james-elicx force-pushed the james/inline-env branch from f8ce991 to a5268a7 Compare December 18, 2024 19:19

james-elicx requested a review from vicb December 18, 2024 19:19

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/bundle-server.ts Outdated Show resolved Hide resolved

vicb reviewed Dec 18, 2024

View reviewed changes

packages/cloudflare/src/cli/build/patches/investigated/compile-env-files.ts Show resolved Hide resolved

vicb approved these changes Dec 18, 2024

View reviewed changes

move locations

dbb57bf

james-elicx merged commit 4341c70 into experimental Dec 18, 2024
6 checks passed

james-elicx deleted the james/inline-env branch December 18, 2024 19:42

inline project env files in worker #181

inline project env files in worker #181

Uh oh!

Conversation

james-elicx commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

pkg-pr-new bot commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vicb commented Dec 16, 2024

Uh oh!

Uh oh!

james-elicx commented Dec 17, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vicb Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

james-elicx Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

vicb Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vicb commented Dec 18, 2024

Uh oh!

Uh oh!

Uh oh!

vicb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

james-elicx commented Dec 15, 2024 •

edited

Loading

changeset-bot bot commented Dec 15, 2024 •

edited

Loading

pkg-pr-new bot commented Dec 15, 2024 •

edited

Loading