Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: regression on variable expansion in dotenv files #1449

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

stefanmaric
Copy link
Contributor

@stefanmaric stefanmaric commented Feb 18, 2025

Overview

PR #978, released in v0.19.10,
broke variable expansion in dotenv files, a Vite built-in feature.

I encountered this while migrating from v0.18.2 to the latest.

Since already defined variables in process.env take presedence,
the call to dotenv-expand by vite won't have any effect. Calling
dotenv-expand ourselves to mimic what vite does restores the lost
functionality.

Vite has a slightly more complex env loading logic, so these changes
won't bring 100% parity, but should be good for most use-cases.

See: https://github.com/vitejs/vite/blob/642d528b7b403eb91c67ff809ffa0fb99a1ff56e/packages/vite/src/node/env.ts

Manual Testing

I've tested this in production already by applying the very same
changes with pnpm patch in my project.

Related Issue

None reported so far (I did not bother to create an issue first).

Copy link

netlify bot commented Feb 18, 2025

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit 98620ad
🔍 Latest deploy log https://app.netlify.com/sites/creative-fairy-df92c4/deploys/67b49686ed672e0008937a71
😎 Deploy Preview https://deploy-preview-1449--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@Timeraa Timeraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aklinker1 wants to keep dependency usage down but I think this is a good thing to keep/have as I'd have a use case myself for this.

Copy link

pkg-pr-new bot commented Feb 18, 2025

Open in Stackblitz

@wxt-dev/auto-icons

npm i https://pkg.pr.new/@wxt-dev/auto-icons@1449

@wxt-dev/i18n

npm i https://pkg.pr.new/@wxt-dev/i18n@1449

@wxt-dev/module-solid

npm i https://pkg.pr.new/@wxt-dev/module-solid@1449

@wxt-dev/module-react

npm i https://pkg.pr.new/@wxt-dev/module-react@1449

@wxt-dev/module-svelte

npm i https://pkg.pr.new/@wxt-dev/module-svelte@1449

@wxt-dev/module-vue

npm i https://pkg.pr.new/@wxt-dev/module-vue@1449

@wxt-dev/storage

npm i https://pkg.pr.new/@wxt-dev/storage@1449

@wxt-dev/unocss

npm i https://pkg.pr.new/@wxt-dev/unocss@1449

wxt

npm i https://pkg.pr.new/wxt@1449

commit: 98620ad

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.35%. Comparing base (e20de4c) to head (98620ad).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1449      +/-   ##
==========================================
- Coverage   81.44%   81.35%   -0.09%     
==========================================
  Files         128      128              
  Lines        6296     6298       +2     
  Branches     1074     1073       -1     
==========================================
- Hits         5128     5124       -4     
- Misses       1153     1159       +6     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stefanmaric
Copy link
Contributor Author

@aklinker1 wants to keep dependency usage down

If it helps put your mind at ease, this change technically doesn't introduce any new dependencies since dotenv-expand is already a transitive dependency via vite.

See: https://github.com/vitejs/vite/blob/60456a54fe90872dbd4bed332ecbd85bc88deb92/packages/vite/package.json#L118

@stefanmaric
Copy link
Contributor Author

Btw, I actually explored using Vite's loadEnv() directly, but I didn't find any way to allow for the browser variants without having to modify vite itself. Maybe something worth sending a PR upstream later, a way to override the dotenv file path variants.

@aklinker1
Copy link
Collaborator

If it helps put your mind at ease, this change technically doesn't introduce any new dependencies since dotenv-expand is already a transitive dependency via vite.

Yeah, adding this dependency makes sense. It'll at least be shared with Vite until there's a major version update and we get out of sync.

@aklinker1
Copy link
Collaborator

Also, looks like there's a type error due to subdependency changes. Try running pnpm i --fix-lockfile to resolve it. Check with pnpm check.

If that doesn't work, I can fix it later.

[PR wxt-dev#978](wxt-dev#978), released in
[v0.19.10](https://github.com/wxt-dev/wxt/releases/tag/wxt-v0.19.10),
broke variable expansion in dotenv files, a Vite built-in feature.

I encountered this while migrating from v0.18.2 to the latest.

Since already defined variables in process.env take presedence,
the call to dotenv-expand by vite won't have any effect. Calling
dotenv-expand ourselves to mimic what vite does restores the lost
functionality.

Vite has a slightly more complex env loading logic, so these changes
won't bring 100% parity, but should be good for most use-cases.

See: https://github.com/vitejs/vite/blob/642d528b7b403eb91c67ff809ffa0fb99a1ff56e/packages/vite/src/node/env.ts
@stefanmaric
Copy link
Contributor Author

Also, looks like there's a type error due to subdependency changes. Try running pnpm i --fix-lockfile to resolve it. Check with pnpm check.

Indeed. I've pushed the package lock fix. 👍

@aklinker1 aklinker1 merged commit a38e3c2 into wxt-dev:main Feb 19, 2025
18 checks passed
Copy link

Thanks for helping make WXT better!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants