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

React 16 upgrade [WIP] #6130

Closed
wants to merge 23 commits into from
Closed

React 16 upgrade [WIP] #6130

wants to merge 23 commits into from

Conversation

tlrobinson
Copy link
Contributor

@tlrobinson tlrobinson commented Oct 9, 2017

Superseded by #6756 #6757 #6758

Upgrade some dependencies to allow upgrading to React 16. Haven't tracked down all dependencies causing warnings, yet.

  • update misc dependencies
  • remove instances of createClass
    • react-sortable
  • remove instances of React.PropTypes
    • react-sortable
    • redux-auth-wrapper
    • redux-form
  • replace react-sortable
  • fix element disappearing while dragging
  • upgrade React to 16
  • fix or catch errors in lifecycle methods
    • catch errors in App as a fallback
    • catch errors in Visualization
    • UserAvatar incorrect merging of styles
    • constrainToScreen shouldn't crash if null element is passed
    • TableInteractive cell measuring code uses ReactDOM.unstable_renderSubtreeIntoContainer
  • replace ReactDOM.unstable_renderSubtreeIntoContainer with ReactDOM.createPortal where possible
    • BodyComponent
    • Modal
      • WindowModal
      • FullPageModal
    • Popover
  • fix tutorial
  • fix hot reloading, if possible

Issues I've found migrating tests to Enzyme v1 and React v16:

  • needs app.update() in some places, in particular after store.waitForActions() in integration tests
  • element.find() doesn't include the element itself
    • for checking classes it's better to do element.hasClass("...") etc
    • possibly related: select.find(Popover) (or children of Popover) no longer works, but app.find(Popover) may

@tlrobinson tlrobinson changed the title React 16 [WIP] React 16 upgrade [WIP] Oct 9, 2017
@tlrobinson
Copy link
Contributor Author

If anyone wants to pick this up I'm not going to be able to work on it for a couple weeks.

@tlrobinson
Copy link
Contributor Author

Ok I've upgraded to React 16, but it still needs a ton of testing due to how React 16 error handling works. Basically React 16 gives you a chance to handle errors thrown in render and other lifecycle methods, but by default it just unmounts the entire app to avoid possibly displaying corrupt data.

So we need to find and fix some bugs that weren't previously a big deal.

We can also add componentDidCatch to certain components to limit the effects of crashes. I added componentDidCatch to the root App component so we can at least display a message apologizing to the user and asking them, to refresh (or hit a button to refresh, or whatever). We can also use this to isolate errors in visualizations better.

Read more here https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html

@tlrobinson
Copy link
Contributor Author

FYI I've moved the relevant parts of the previous branch to a new branch from master since it was too difficult to merge all the package changes.

@tlrobinson tlrobinson self-assigned this Jan 12, 2018
@tlrobinson
Copy link
Contributor Author

Superseded by #6756 #6757 #6758

@tlrobinson tlrobinson closed this Jan 24, 2018
@tlrobinson tlrobinson deleted the react-16-wip branch January 24, 2018 08:59
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.

None yet

1 participant