Skip to content

Conversation

@jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Jul 23, 2024

What are the relevant tickets?

Closes #1057

Description (What does it do?)

  • Moves all env vars to APP_SETTINGS.
  • Renames APP_SETTINGS properties to match variable names.
  • Removes use of the Webpack EnvironmentPlugin and the process object in frontend code.
  • All values checked and set via envalid.

Breaking change: We previously tolerated MITOPEN_AXIOS_WITH_CREDENTIALS=True on the environment. This is now checked by envalid, which required that booleans are provided in lowercase, 'true'.

How can this be tested?

App builds, tests pass and things run.

Additional Context

We were using two mechanisms to provide build time environment variables to the frontend code. An APP_SETTINGS object provided by the Webpack DefinePlugin and others provided by the EnvironmentPlugin, which writes onto process.env in the front end code (through string replacement, not the actual process object).

The latter has often been the preferred method as it provides consistency between the build and for tests running in a Node.js process, however the process object is specific to Node.js and doesn't really belong in front end code. Webpack 5 removed its polyfill for the process object and recommends it be avoided.

… to APP_SETTINGS, properties renamed to align, supplied via envalid.
@jonkafton jonkafton marked this pull request as draft July 23, 2024 21:12
@jonkafton jonkafton marked this pull request as ready for review July 24, 2024 18:06
@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Jul 24, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

@jonkafton Shouldn't all instances of process.env.whatever be replaced with APP_SETTINGS now? Except process.env.NODE_ENV, which is special—webpack strips out blocks that are process.env.NODE_ENV === "development".

I'm seeing several remaining process.env.SITE_NAME references. Could you update these and double check all other process.env. instances?

Site name missing:
Screenshot 2024-07-25 at 10 02 21 AM

}),
CKEDITOR_UPLOAD_URL: str({
desc: "Location of the CKEditor uploads handler",
default: "https://35904.cke-cs.com/easyimage/upload/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure we should commit this. It's not secret, since it is used on the frontend, but it is env specific.

Could add a dev default of a fake url 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've set it to an empty string, which disables the file upload option according to their docs.

@jonkafton
Copy link
Contributor Author

jonkafton commented Jul 25, 2024

I'm seeing several remaining process.env.SITE_NAME references. Could you update these and double check all other process.env. instances?

Good spot, I missed that one somehow - now replaced.

Shouldn't all instances of process.env.whatever be replaced with APP_SETTINGS now? Except process.env.NODE_ENV, which is special—webpack strips out blocks that are process.env.NODE_ENV === "development".

Yes, all instances in the app code should be removed. Ok to use in tests, Storybook and the webpack config itself. We have dynamic env var names for the Posthog feature flags, for example.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍

@jonkafton jonkafton merged commit bb2f2be into main Jul 26, 2024
This was referenced Jul 26, 2024
@rhysyngsun rhysyngsun deleted the jk/1057-env-vars branch February 7, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consistent environment variables for the front end build

3 participants