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

Bugfix: setEnvVar #32

Merged
merged 1 commit into from
Oct 26, 2020
Merged

Bugfix: setEnvVar #32

merged 1 commit into from
Oct 26, 2020

Conversation

jwdotjs
Copy link
Contributor

@jwdotjs jwdotjs commented Oct 25, 2020

If setEnvVar was used twice in a row, the first env var key/value pairs
would be dropped and the second would be kept. This was due to us
pulling environment from React's state and the re-render cycle
doesn't occur quick enough for environment to have changes from
update 1 of 2.

By pulling directly from localStorage, which is synchronous, it
guarantees the updates took effect

If setEnvVar was used twice in a row, the first env var key/value pairs
would be dropped and the second would be kept. This was due to us
pulling `environment` from React's state and the re-render cycle
doesn't occur quick enough for `environment` to have changes from
update 1 of 2.

By pulling directly from localStorage, which is synchronous, it
guarantees the updates took effect
Copy link
Contributor

@chrisheninger chrisheninger left a comment

Choose a reason for hiding this comment

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

Good fix– timing issues around managing state can be a huge pain.

There's probably some other optimizations we could do around this piece in the future– seems like a spot with a lot happening. (Global app context, things being set/read from localstorage, and those localstorage items also having React state tracking them and that result getting passed down via context.) It's a common pattern that we also see in our apps. 👍

@chrisheninger chrisheninger merged commit b8dc03b into master Oct 26, 2020
@chrisheninger chrisheninger deleted the bugfix/set-env-var branch October 26, 2020 14:45
@jwdotjs
Copy link
Contributor Author

jwdotjs commented Oct 26, 2020

Seems like a spot with a lot happening

Totally! I also didn't know how to properly leverage things like useMemo back during the initial implementation so I definitely think there are a lot optimizations to be had in this project 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants