Guard import.meta.env in ConfigProvider.fromEnv - #6570
Conversation
🦋 Changeset detectedLatest commit: b097ad8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesConfigProvider environment loading
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/effect/test/ConfigProvider.test.ts (1)
194-208: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for unsupported
import.metaenvironments.This test only verifies the process-environment fallback. Add a build/runtime fixture for an environment that cannot access
import.meta, so the new guard is validated against the failure mode described in the PR objective.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/test/ConfigProvider.test.ts` around lines 194 - 208, Add regression coverage alongside the existing default-environment test for ConfigProvider.fromEnv that executes it in a runtime/build fixture where import.meta is unavailable. Assert the provider still uses process.env without throwing, while preserving the current environment restoration and success expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/effect/test/ConfigProvider.test.ts`:
- Around line 194-208: Add regression coverage alongside the existing
default-environment test for ConfigProvider.fromEnv that executes it in a
runtime/build fixture where import.meta is unavailable. Assert the provider
still uses process.env without throwing, while preserving the current
environment restoration and success expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9c0f5be-8963-4948-848b-ea8934422b63
📒 Files selected for processing (2)
packages/effect/src/ConfigProvider.tspackages/effect/test/ConfigProvider.test.ts
tim-smart
left a comment
tim-smart
left a comment
There was a problem hiding this comment.
This won't fix the issue as it fails during a module analysis step, not at runtime.
|
Thanks, that makes sense. I updated the branch so Local check: |
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
…onfigprovider-import-meta-env-6358 # Conflicts: # packages/effect/src/ConfigProvider.ts
|
已推送 |
2 participants
Fixes #6358
ConfigProvider.fromEnv()currently readsimport.meta.envwhile building the default environment. Some runtimes can throw whenimport.metais accessed, so this guards that lookup and falls back toprocess.env/ an empty Vite env instead.Tested:
pnpm --filter effect checkpnpm --filter effect exec vitest run test/ConfigProvider.test.ts -t "fromEnv" --config vitest.config.tsSummary by CodeRabbit
ConfigProvider.fromEnv()to resolve its default environment fromprocess.env(when available) instead of using anyimport.meta.envfallback.fromEnvguidance to match the new default behavior and clarify how to passenvin custom runtimes.process.env, and that an explicitenvoption overrides the default.import.meta.