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

ADD defaults & check for valid NEAR_WALLET_ENV #2299

Merged
merged 17 commits into from
Jan 11, 2022
Merged

Conversation

esaminu
Copy link
Contributor

@esaminu esaminu commented Dec 9, 2021

  • Check for valid NEAR_WALLET_ENV in module scope and throw if invalid*
  • Parametrize bundle configs by NEAR_WALLET_ENV
  • Set overridable defaults per NEAR_WALLET_ENV
  • Remove usage of REACT_APP_IS_MAINNET and replace with NEAR_WALLET_ENV conditional
  • Add eslint rule to disallow usage of process.env outside of config files

*This is a breaking change in all environments until NEAR_WALLET_ENV is correctly set to one of these values.

Closes #2291

@render
Copy link

render bot commented Dec 9, 2021

@esaminu esaminu removed the request for review from MaximusHaximus December 9, 2021 23:10
@esaminu esaminu marked this pull request as draft December 9, 2021 23:10
@esaminu esaminu marked this pull request as ready for review December 9, 2021 23:34
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Looking at all of the duplicated OR cases with process.env references repeated in every file, it feels untenable to leave so many places with duplicated references in place.

Let's do this:

  1. Create a 'global' hash of all config keys and their associated references to process.env.XXX_YYYYY; this should exist in a central file because the references to process.env are always the same, no matter what value is in NEAR_WALLET_ENV.
  2. When defining per-environment defaults, don't include the reference to process.env. Instead, let's merge the values from the 'global' hash of all config keys that have been loaded from the process environment with the defaults for the current environment, using Lodash.defaults(), where our 'global' hash populated from the process.env is the first argument and the current environment's hash is the second. (see https://lodash.com/docs/4.17.15#defaults)
  3. As a result of # 1 and # 2 above, if any config value's associated process.env mapping has a value, it will always take precedence over any per-environment defaults (they will truly be defaults). If nothing is defined in the process.env key associated with a particular config entry, then the per-environment default will be applied.

HMU directly with any questions/concerns about this :). I think it will be really good to have all of our process.env parsing in a centralized file and have all of our per-environment config files just be a raw hash of values with no process.env fallback logic -- just default values per key.

packages/frontend/src/ExampleFlag.js Show resolved Hide resolved
packages/frontend/.eslintrc.js Show resolved Hide resolved
packages/frontend/src/config/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Nice work! Just a few minor comments inline 🙂

packages/frontend/src/config/index.js Outdated Show resolved Hide resolved
packages/frontend/src/config/envParsers.js Outdated Show resolved Hide resolved
packages/frontend/src/config/envParsers.js Outdated Show resolved Hide resolved
@andy-haynes
Copy link
Contributor

New changes look good to me but bundling is failing because NEAR_WALLET_ENV isn't set in the Travis CI environment. @MaximusHaximus does NEAR_WALLET_ENV need to be set in .travis.yml?

Since this is a breaking change do we also need a value for this in netlify.toml?

@esaminu
Copy link
Contributor Author

esaminu commented Dec 16, 2021

@andy-haynes I'll be adding functionality to compute NEAR_WALLET_ENV for each ci environment. Converting to draft for now.

@esaminu esaminu marked this pull request as draft December 16, 2021 19:48
@andy-haynes
Copy link
Contributor

@andy-haynes I'll be adding functionality to compute NEAR_WALLET_ENV for each ci environment. Converting to draft for now.

Perfect, thanks for the clarification!

@esaminu esaminu marked this pull request as ready for review December 20, 2021 18:53
Copy link
Contributor

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Looking good, mostly minor comments but I still think we need to find a way to ensure NEAR_WALLET_ENV is set in the mainnet* environments as well as testnet_STAGING (which currently just uses testnet). I'm happy to sync up when we have some good schedule overlap to go over the feedback and work out a plan for testnet_STAGING.

packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/src/config/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Added comments in-line -- in brief, let's totally remove testnet_STAGING for now and eliminate any fuzzy fallbacks :) Saw a couple of spots that we should be unpacking env vars that I also mentioned in-line.

packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/src/config/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/src/config/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/ci/configFromEnvironment.js Outdated Show resolved Hide resolved
packages/frontend/src/config/configFromEnvironment.js Outdated Show resolved Hide resolved
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Just one thing re: always removing whitespace in the parse comma helper function

Also, Andy's comment re: https://github.com/near/near-wallet/blob/master/packages/frontend/ci/ParcelBundler.js#L13 is accurate -- please remove the coercion there :). Then we can get this sucker landed! I'm keen to get this merged today (Jan 11th) if possible.

packages/frontend/src/config/envParsers.js Outdated Show resolved Hide resolved
Copy link
Contributor

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

😎

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 legit -- nice work folks! We're really making some progress on our snarled and confusing environments at this point!

@MaximusHaximus MaximusHaximus merged commit 804a492 into master Jan 11, 2022
@MaximusHaximus MaximusHaximus deleted the issue-2291 branch January 11, 2022 22:56
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.

NEAR_WALLET_ENV should control default env var values
3 participants