Skip to content

Conversation

@deinakings
Copy link
Contributor

@deinakings deinakings commented Oct 16, 2019

What kind of change does this PR introduce?

This is a refactor related to #2621
@Saeris @christianalfoni

What is the current behavior?

EnvironmentVariables component was using inject and hooksObserver from app/componentConnectors.

What is the new behavior?

  • Refactored to use useOvermind from app/overmind
  • Changed extension of the file from .js to .tsx

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

yarn typecheck
yarn lint
yarn test

I got some warnings for yarn lint but no errors.

  1. Then added the jwt cookie
  2. Created a new sandbox container
  3. Opened "Server Control Panel"
  4. Added a test environment variable:

Screen Shot 2019-10-16 at 9 52 33 AM

Checklist

  • Documentation N/A
  • Testing
  • Ready to be merged
  • Added myself to contributors table

};

const envVars = store.editor.currentSandbox.environmentVariables;
const envVars: any = currentSandbox.environmentVariables;
Copy link
Member

Choose a reason for hiding this comment

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

Why would this become an any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

environmentVariables is an Array so this was giving me type errors since in the code below is trying to use toJson method and it doesn’t exits in an array type
Anyway that is fixed now with the changes @christianalfoni added :)

@christianalfoni
Copy link
Contributor

Great stuff here @deinakings! I just added a PR that fixes some internal typings and also some cleanup related to the old solution we had. If you merge that in this PR should be ready to go 😄

deinakings#1

@deinakings
Copy link
Contributor Author

deinakings commented Oct 17, 2019

Thanks @christianalfoni! Your PR is merged! 🚀

@SaraVieira
Copy link
Contributor

Thank you! Merged!

@SaraVieira SaraVieira merged commit d1c88a1 into codesandbox:master Oct 19, 2019
@MichaelDeBoey MichaelDeBoey added 🔨 Refactor 🧠 Overmind Indicates that this is related to the app's State Management labels Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🧠 Overmind Indicates that this is related to the app's State Management 🔨 Refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants