Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@mleah
Copy link

@mleah mleah commented Dec 1, 2017

Description

This is a PR to fix an issue we are seeing on the MetricsOne frontend.

This issue is currently up in a PR to be fixed on the redux-devtools-inspector repo. However, it has been up since May, and there is no resolution in sight.

There is also an issue up for the original repo.

Testing Steps

In order to test this, you will need to pull this branch on redux-devtools-inspector into your FE.

  1. While on the FE, run yarn remove redux-devtools-inspector.
  2. After the command successfully finishes, run yarn add github:detroit-labs/redux-devtools-inspector#LNG-2305-Fixing-typechecking-order-issue
  3. Once this command successfully finished, start up the FE and the MW locally.
  4. Ensure that the FE compiles correctly.
  5. Click around through the FE to make sure the app works.
  6. If you were not having the issue happen, this is the full test. Please see me for more info if you have questions..
  7. If you were having the issue, this should resolve it 🤞 .

Once this PR is approved and merged in, I will put up a corresponding FE PR.

node_modules
npm-debug.log
.DS_Store
lib
Copy link
Author

Choose a reason for hiding this comment

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

Removed the lib from gitignore. There are build issues when trying to pull this into the FE repo . The lib isn't being properly built when being included in the FE, so including the lib here is a quick fix for this issue.

@@ -1,3 +1,5 @@
// @noflow
Copy link
Author

Choose a reason for hiding this comment

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

Flow tests for this file were failing, even on master of the fork 😕

This addition stops flow (a typing library) from trying to type this library.

val = val[val.length === 2 ? 1 : 0];
}

const type = getType(val);
Copy link
Author

@mleah mleah Dec 1, 2017

Choose a reason for hiding this comment

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

This is the change needed to (hopefully) fully fix the issue with the FE.

@SlaunchaMan SlaunchaMan self-requested a review December 1, 2017 20:56
Copy link

@SlaunchaMan SlaunchaMan left a comment

Choose a reason for hiding this comment

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

LGTM! @hoffmanktz, this is ready for QA!

@hoffmanktz
Copy link

Not throwing any errors in the console and everything is working as expected. LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants