Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: add 'PREACT_APP_' prefixed env vars automatically & pickup .env file #1671

Merged
merged 7 commits into from Apr 11, 2022

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Mar 14, 2022

What kind of change does this PR introduce?

feature

Did you add tests for your changes?

Yes

Summary

This is something that's asked about fairly often and I figure it should become better supported for how low maintenance it is for us.

Any env vars prefixed with PREACT_APP_ will now automatically be added to DefinePlugin. This matches what CRA does and also (mostly) matches what preact-cli-plugin-env-vars does. This is still a fairly popular plugin, getting ~1/10 of the downloads per week that preact-cli itself is.

I added a note in the changeset to give credit for this functionality in the past. Hopefully that's appropriate; no code is actually shared.

.env files in the user's project root are also now automatically consumed. Added dotenv for handling this.

Does this PR introduce a breaking change?

Shouldn't be, no.

@rschristian rschristian requested a review from a team as a code owner March 14, 2022 09:14
@changeset-bot
Copy link

changeset-bot bot commented Mar 14, 2022

🦋 Changeset detected

Latest commit: 4fe11a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Minor

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

@rschristian
Copy link
Member Author

One thing that came up is the question of whether the .env file path should be a flag or not. Can't say I have much of an opinion either way.

Comment on lines 109 to 115
let cwd = resolve(argv.cwd);
require('dotenv').config({ path: resolve(cwd, '.env') });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let cwd = resolve(argv.cwd);
require('dotenv').config({ path: resolve(cwd, '.env') });
require('dotenv').config();

Wouldn't this work as well?
https://github.com/motdotla/dotenv#path

Copy link
Member Author

Choose a reason for hiding this comment

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

In normal circumstances, sure.

However, that would break in our test suite (and probably mono-repos too): procces.cwd() (which dotenv uses) would differ from the project root.

I ran into that issue and moved the dotenv setup here accordingly.

Sorry, that could've done with a comment.

Edit: Added comments to both usages.

@ForsakenHarmony ForsakenHarmony changed the title Feat: Add 'PREACT_APP_' prefixed env vars automatically & pickup .env file feat: add 'PREACT_APP_' prefixed env vars automatically & pickup .env file Mar 29, 2022
@ForsakenHarmony
Copy link
Member

One thing that came up is the question of whether the .env file path should be a flag or not.

Could always add that later on

@rschristian
Copy link
Member Author

Hm something in that watch test is badly misbehaving and getting the CI stuck in a loop. Will need to look into that tomorrow, though nothing stands out at the moment.

@ForsakenHarmony ForsakenHarmony merged commit 8d3bd42 into master Apr 11, 2022
@ForsakenHarmony ForsakenHarmony deleted the feat/env-vars branch April 11, 2022 15:35
@preact-bot preact-bot mentioned this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants