fix(start-plugin-core): load env files in config hook to ensure mode-specific files are merged #6202
Conversation
WalkthroughModified the load-env-plugin to use the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
View your CI Pipeline Execution ↗ for commit 6e1159b
☁️ Nx Cloud last updated this comment at |
hyungwookchoi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/start-plugin-core/src/load-env-plugin/plugin.ts (1)
20-23: Consider documenting the merge order semantics.The spread order
...userConfig.define, ...definemeans environment-loaded defines will override user-provided defines if they share the same keys. While unlikely (user defines and env var keys rarely conflict), this behavior could be surprising.Consider adding a comment to clarify the intended precedence:
🔎 Suggested documentation
return { define: { + // User-provided defines are applied first, then env-based defines override. + // This ensures environment variables take precedence as expected. ...userConfig.define, ...define, }, }packages/start-plugin-core/tests/load-env/load-env.test.ts (1)
27-128: Consider adding edge case tests for robustness.While the current test coverage is solid, consider adding tests for these edge cases:
- Missing .env files: Verify graceful handling when no env files exist
- Empty .env files: Ensure empty files don't cause issues
- Special characters in values: Test env values containing quotes, newlines, or other special characters
- Invalid .env syntax: Verify behavior with malformed env files
These tests would increase confidence in the plugin's robustness, especially since
loadEnvfrom Vite handles these scenarios internally.🔎 Example edge case tests
test('should handle missing .env files gracefully', () => { // No .env files created const plugin = loadEnvPlugin() const configHook = (plugin as any).config const result = configHook({ root: testDir }, { mode: 'development' }) expect(result).toHaveProperty('define') expect(Object.keys(result.define).length).toBe(0) }) test('should handle env values with quotes', () => { writeFileSync(join(testDir, '.env'), 'TEST_QUOTED="value with \\"quotes\\""') const plugin = loadEnvPlugin() const configHook = (plugin as any).config const result = configHook({ root: testDir }, { mode: 'development' }) expect(process.env.TEST_QUOTED).toBeDefined() expect(result.define['process.env.TEST_QUOTED']).toContain('quotes') })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-plugin-core/src/load-env-plugin/plugin.tspackages/start-plugin-core/tests/load-env/load-env.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-plugin-core/tests/load-env/load-env.test.tspackages/start-plugin-core/src/load-env-plugin/plugin.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/tests/load-env/load-env.test.tspackages/start-plugin-core/src/load-env-plugin/plugin.ts
🧬 Code graph analysis (2)
packages/start-plugin-core/tests/load-env/load-env.test.ts (1)
packages/start-plugin-core/src/load-env-plugin/plugin.ts (1)
loadEnvPlugin(4-27)
packages/start-plugin-core/src/load-env-plugin/plugin.ts (1)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (1)
mode(313-315)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (5)
packages/start-plugin-core/src/load-env-plugin/plugin.ts (2)
8-24: Implementation correctly addresses the environment loading issue, but consider type safety.The new
confighook implementation properly loads environment variables based on mode and generates the necessary define mappings. The approach of usingloadEnv(mode, root, '')with an empty prefix ensures all environment variables are loaded, which aligns with the PR objectives to fix issue #6052.However, there's a type safety concern with the
definevariable on line 14.Type safety issue:
Line 14 declares
defineasRecord<string, string>, butuserConfig.definecan contain values of typeanyaccording to Vite's configuration types. This could lead to type inconsistencies when merging.🔎 Suggested fix for type safety
- const define: Record<string, string> = {} + const define: Record<string, any> = {} for (const [key, value] of Object.entries(env)) { define[`process.env.${key}`] = JSON.stringify(value) }As per coding guidelines, TypeScript strict mode requires extensive type safety for all code.
12-12: Verify the implications of mutating global process.env.The plugin mutates
process.envglobally withObject.assign(process.env, env). While this is likely intentional to make environment variables accessible throughout the Node.js process, it could have side effects:
- Multiple plugin instances or repeated calls could cause unexpected env var accumulation
- Tests or other plugins may be affected by this global state mutation
- In watch mode, env vars may persist across rebuilds when they shouldn't
Please verify:
- Is the global mutation of
process.envnecessary, or should env vars be exposed only through thedefineconfig?- Should the plugin track which env vars it added to enable cleanup?
- Are there any scenarios where this could cause issues in development/watch mode?
packages/start-plugin-core/tests/load-env/load-env.test.ts (3)
8-25: Excellent test setup with thorough environment cleanup.The
beforeEachandafterEachhooks properly isolate tests by:
- Creating unique temporary directories per test
- Cleaning up filesystem changes
- Restoring
process.envto its original state by removing added keys and reassigning original valuesThis prevents test pollution and ensures reliable test execution.
27-128: Comprehensive test coverage validates the plugin behavior.The test suite thoroughly validates:
- Base
.envfile loading and define generation (lines 27-37)- Correct precedence of mode-specific files over base files (lines 39-57, 59-89)
- Mode-aware loading for both development and production (lines 59-89)
- Define config generation with proper JSON stringification (lines 91-100)
- Preservation of user-provided define mappings (lines 102-118)
- Fallback behavior when root is unspecified (lines 120-127)
These tests validate the fix for issue #6052 where environment variables weren't being merged correctly.
31-31: Type assertion is necessary for testing internal plugin hooks.The pattern
(plugin as any).configis used throughout to access the internalconfighook for testing. This is acceptable for test code since:
- Vite plugins don't expose their hooks as part of the public API
- Direct hook testing provides more granular verification than integration tests
- The tests validate both side effects (process.env) and return values (define)
No changes needed, but be aware this creates coupling to the plugin's internal structure.
Also applies to: 50-50, 68-68, 84-84, 95-95, 106-106, 122-122
1 participant
fixes #6052
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.