Iterate on .env files: make the behavior override
#10094
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tobbe
approved these changes
Mar 2, 2024
jtoar
added a commit
that referenced
this pull request
Mar 2, 2024
Follow up to #10093. After discussing with @orta, the `--add-env-files` flag being additive instead of overriding wasn't a sticking point in the original implementation, and the more feedback I get, it seems like most people expect later files to override earlier ones, so let's switch the behavior. In tandem, at first I tried to rename the flag back to just `--env-file` or `--env-files`, but I'm afraid both give this cryptic error: ``` redwood-app % yarn rw --env-file prod node: prod: not found # Or `yarn rw --env-files prod`, which also gives this error, # which I didn't know node would also process? Their docs only say `--env-file`... ``` The above was with my framework changes. I tried it again without my framework changes and the error persists. As far as I can tell, `rw` never executes and it seems like the Node.js binary itself is evaluating the `--env-file` flag. It supports `--env-file` now, and expects it to be a full path (so `.env.prod`). But this isn't how I thought node processes `process.argv` at all... I thought that all flags would've been passed to the script (here, `rw`) for processing. But maybe not, and if so that means we can just pass options to node via `yarn rw --my-node-option`. Which I still don't quite believe and if it is true, then I'm not sure what to make of yet cause people have been wanting to pass node options for a while and I always thought `NODE_OPTIONS="--my-node-option" yarn rw ...` was the only way. So TL;DR, that's why I've left the name here and I'd like to keep that friction point out of the scope of this PR being considered mergeable.
dac09
added a commit
to dac09/redwood
that referenced
this pull request
Mar 4, 2024
…sc-build * 'feat/rsc-build' of github.com:dac09/redwood: (32 commits) RSC: ensureProcessDirWeb() (redwoodjs#10108) Comment on ensureProcessDirWeb() Remove redundant cwd check RSC: Extract webpack shims into their own file (redwoodjs#10107) RSC: Remove completed TODO comment RSC: Babel react plugin not needed for analyze phase (redwoodjs#10106) Remove handled TODO RSC: runFeServer: wrap RSC code with `if (rscEnabled)` (redwoodjs#10105) Only comment about swc in one place Remove duplicated comment (exists further down) RSC: Update comments, naming etc based on Danny's input (redwoodjs#10104) RSC: Rename to buildRscClientAndServer (redwoodjs#10103) RSC: Rename to rscBuildForServer, and tweak some comments (redwoodjs#10102) SSR: Extract buildForStreamingServer function (redwoodjs#10099) chore(unit-tests): Silence middleware error logging (redwoodjs#10097) Iterate on `.env` files: make the behavior override (redwoodjs#10094) RSC: smoke-tests: Compare text, not html (redwoodjs#10098) paths.ts: Move helper to esm section More comment formatting getMergedConfig comment format ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #10093. After discussing with @orta, the
--add-env-filesflag being additive instead of overriding wasn't a sticking point in the original implementation, and the more feedback I get, it seems like most people expect later files to override earlier ones, so let's switch the behavior.In tandem, at first I tried to rename the flag back to just
--env-fileor--env-files, but I'm afraid both give this cryptic error:The above was with my framework changes. I tried it again without my framework changes and the error persists. As far as I can tell,
rwnever executes and it seems like the Node.js binary itself is evaluating the--env-fileflag. It supports--env-filenow, and expects it to be a full path (so.env.prod). But this isn't how I thought node processesprocess.argvat all... I thought that all flags would've been passed to the script (here,rw) for processing. But maybe not, and if so that means we can just pass options to node viayarn rw --my-node-option. Which I still don't quite believe and if it is true, then I'm not sure what to make of yet cause people have been wanting to pass node options for a while and I always thoughtNODE_OPTIONS="--my-node-option" yarn rw ...was the only way.So TL;DR, that's why I've left the name here and I'd like to keep that friction point out of the scope of this PR being considered mergeable.