fix(config): apply $production/$development layers when NODE_ENV is unset#4066
Open
jlgrimes wants to merge 3 commits intonitrojs:mainfrom
Open
fix(config): apply $production/$development layers when NODE_ENV is unset#4066jlgrimes wants to merge 3 commits intonitrojs:mainfrom
jlgrimes wants to merge 3 commits intonitrojs:mainfrom
Conversation
|
@jlgrimes is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds an envName computed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jlgrimes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/config-loader-env.test.ts (1)
60-61: Consider verifying base layer is preserved alongside env layer.The fixture defines a base
/baseroute. Adding an assertion for it would confirm that env-specific layers are merged correctly without losing base configuration.💡 Optional enhancement
expect(options.routeRules["/prod"]?.headers?.["x-env"]).toBe("production"); expect(options.routeRules["/dev"]).toBeUndefined(); + expect(options.routeRules["/base"]?.headers?.["x-env"]).toBe("base"); }); it("applies $development when NODE_ENV is unset and dev=true", async () => { // ... expect(options.routeRules["/dev"]?.headers?.["x-env"]).toBe("development"); expect(options.routeRules["/prod"]).toBeUndefined(); + expect(options.routeRules["/base"]?.headers?.["x-env"]).toBe("base"); });Also applies to: 71-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/config-loader-env.test.ts` around lines 60 - 61, Add assertions that the base route from the fixture is still present after env layering: check options.routeRules["/base"] exists and its original properties (e.g., headers, upstreams, or whatever the fixture defines) match expected values to ensure the env layer merged rather than replaced the base layer; update both places where routeRules for env tests are asserted (the blocks that currently check "/prod"/"/dev") to include these "/base" assertions referencing options.routeRules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/config-loader-env.test.ts`:
- Around line 60-61: Add assertions that the base route from the fixture is
still present after env layering: check options.routeRules["/base"] exists and
its original properties (e.g., headers, upstreams, or whatever the fixture
defines) match expected values to ensure the env layer merged rather than
replaced the base layer; update both places where routeRules for env tests are
asserted (the blocks that currently check "/prod"/"/dev") to include these
"/base" assertions referencing options.routeRules.
Author
|
Added the follow-up test assertions suggested in review:
Commit: 1940d6b |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/config-loader-env.test.ts`:
- Around line 45-46: The afterEach cleanup currently does "process.env.NODE_ENV
= originalNodeEnv" which coerces undefined into the string "undefined"; change
the teardown (the afterEach that references originalNodeEnv and
process.env.NODE_ENV) to conditionally restore or remove NODE_ENV: if
originalNodeEnv is defined set process.env.NODE_ENV back to it, otherwise delete
process.env.NODE_ENV so the environment variable is truly unset for subsequent
tests.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/config-loader-env.test.ts (1)
57-81: Consider adding one test for explicitopts.c12.envNameoverride precedence.Since this PR also preserves explicit override behavior, a focused case (e.g.,
dev: false+c12.envName: "development") would lock in that contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/config-loader-env.test.ts` around lines 57 - 81, Add a test that asserts explicit opts.c12.envName takes precedence over inferred NODE_ENV/dev logic: in the same suite call loadOptions({ rootDir: await createFixtureConfig(), dev: false, c12: { envName: "development" } }) (or similar shape used by loadOptions) and assert that options.routeRules["/dev"] has x-env "development", options.routeRules["/prod"] is undefined, and base still applies; place this alongside the existing tests so it exercises the loadOptions function and the explicit c12.envName override path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/config-loader-env.test.ts`:
- Around line 57-81: Add a test that asserts explicit opts.c12.envName takes
precedence over inferred NODE_ENV/dev logic: in the same suite call
loadOptions({ rootDir: await createFixtureConfig(), dev: false, c12: { envName:
"development" } }) (or similar shape used by loadOptions) and assert that
options.routeRules["/dev"] has x-env "development", options.routeRules["/prod"]
is undefined, and base still applies; place this alongside the existing tests so
it exercises the loadOptions function and the explicit c12.envName override
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a5ad140-0e5b-4934-ac1f-283b41867a59
📒 Files selected for processing (1)
test/unit/config-loader-env.test.ts
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🔗 Linked issue
Resolves #3052
❓ Type of change
📚 Description
Nitro uses c12 for config loading, but when NODE_ENV is unset it currently forwards no envName, so c12 skips $production and $development layers entirely.
This causes production deployments (e.g. Cloudflare Pages) to ignore $production config blocks unless NODE_ENV is manually set.
What changed
Validation