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

fix(dotenv-loading): Fixes dotenv loading regression #4334

Closed
wants to merge 2 commits into from

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Feb 2, 2022

This PR does the following

1. Removes the dotenv-webpack plugin.

I'm not sure why this was here, probably legacy reasons, but having this plugin here meant we weren't handling env vars on the web side, the way our documentation said we did.

Example: Accessing "unsafe" variables

# redwood.toml
+includeEnvironmentVariables = ['BAZINGA']

And somewhere in the web code we do the following:

console.log(process.env.BAZINGA) // prints values of BAZINGA ✅ comes through, great

console.log(process.env.DATABASE_URL) // prints value of DATABASE_URL ❌ this is not expected behaviour

2. Prevents erroring out on the website with process.env is undefined errors, by instantiting all the values defined in includeEnvironmentVariables

This was the regression in v0.43

Example:

# redwood.toml
+includeEnvironmentVariables = ['ENV_1', 'ENV_2']
#.env
-ENV_1=value
-ENV_2=value

And somewhere in the web code we do:

console.log('env1', process.env.ENV_1)
console.log('env2', process.env.ENV_2)
  • in v0.42: it would print:
env1 MISSING_ENV_VAR // ❌ unexpected, but acceptable, behaviour
env2 MISSING_ENV_VAR
  • in v0.43 it would error withprocess.env is undefined
    ❌ unexpected behaviour

  • After this fix:

env1 undefined // ✅ expected behaviour
env2 undefined

3. Adds jest config for rwjs/core

We had a test in the @redwoodjs/core but no jest config, and no script to actually run it. :S Adds jest stuff back so the test actually runs (also updated the test)

@dac09
Copy link
Collaborator Author

dac09 commented Feb 2, 2022

@thedavidprice this PR solves the regression we have in 0.43 and makes the env var handling inline with expected behaviour.

I've also taken storybook for a run, all seems ok!

@dac09 dac09 added the release:fix This PR is a fix label Feb 2, 2022
@thedavidprice
Copy link
Contributor

@dac09 I want to spend more time discussing this before proceeding as I have questions about some production scenarios.

Also, I'd like to spend more time revising the test (that wasn't being used!). I ran the test against the original webpack config in main and couldn't get it to fail.

If we do remove DotEnv, we'll need to revise the Docs, which frequently mention DotEnv.

For the v0.44 release, I'm going to revert the config via #4363 We still need it for CI, so I'll revert the revert after 0.44 is released to fix CI until this PR is complete.

Copy link
Collaborator Author

dac09 commented Feb 3, 2022

Hey DP, yes I imagine the test wouldn't fail before. It just wasn't being run.

We do still use dotenv plenty, just that we're not using that specific plugin in order to do what we say we do in the docs!

Comment on lines -172 to -176
new Dotenv({
path: path.resolve(redwoodPaths.base, '.env'),
silent: true,
ignoreStub: true, // FIXME: this might not be necessary once the storybook webpack 4/5 stuff is ironed out. See also: https://github.com/mrsteele/dotenv-webpack#processenv-stubbing--replacing
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion:

  • ignoreStub: true is no longer needed for Storybook; I've already removed in main
  • Let's add this back and not change config in this PR
  • but do keep tests; we need 'em!

@dac09
Copy link
Collaborator Author

dac09 commented Feb 14, 2022

Closing this PR, as now resolved with the stubbing setting.

However, I still hold position that we're note doing what our docs say in relation to includeEnvironmentVariables. To be discussed later!

@dac09 dac09 closed this Feb 14, 2022
@thedavidprice thedavidprice reopened this Feb 14, 2022
@thedavidprice
Copy link
Contributor

@dac09 I thought we had discussed adding the tests regardless (since they're currently not run)?

Copy link
Collaborator Author

dac09 commented Feb 14, 2022

Yeah created another PR, see here: #4461

@thedavidprice
Copy link
Contributor

🤘

Perfect. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

None yet

3 participants