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

Upgrade React to v16 #1685

Merged
merged 13 commits into from Apr 2, 2018

Conversation

Projects
None yet
2 participants
@jseppi
Contributor

jseppi commented Mar 30, 2018

This upgrades the Data Explorer app to the latest version of React, v16, which has all kinds of neat features (blog).

As part of this upgrade, I also had to upgrade a number of other dependencies:

  • enzyme and enzyme-to-json
  • rc-slider
  • react-dom
  • react-redux
  • react-transition-group

and make a few small changes to how jest and enzyme work together (see data-explorer/tests/setup.js).

Overall the upgrade was fairly painless. All tests are passing, and I don't notice any differences when manually playing around with the Data Explorer interface, but please do some local exercise during your review, @toolness.

@jseppi jseppi self-assigned this Mar 30, 2018

James Seppi added some commits Mar 30, 2018

James Seppi
James Seppi
fix tooltipster tests
shallow rendering resulted in an undefined `ref` within the component, so only do `mount`ed rendering for the test

@jseppi jseppi force-pushed the upgrade-react-v16 branch from dbbe32a to 8ba8bd0 Mar 30, 2018

@jseppi jseppi changed the title from [WIP] Upgrade React to v16 to Upgrade React to v16 Mar 30, 2018

@jseppi jseppi requested a review from toolness Mar 30, 2018

James Seppi
upgrade babel and webpack
also switch to babel-preset-env, which will target the latest 2 browser versions

@jseppi jseppi referenced this pull request Mar 30, 2018

Merged

Upgrade babel, webpack, and node #1687

2 of 2 tasks complete

James Seppi added some commits Mar 30, 2018

James Seppi
James Seppi
@toolness

Nice! This looks fab. I played around with it on my machine and it works nicely too.

Just a few notes regarding the release notes:

  • The JavaScript Environment Requirements mention that React 16 depends on Map, Set, and requestAnimationFrame. I forget what version of IE we support at this point, but we should either make sure we still support that version, or make sure it's OK to drop support for it...

  • The release notes also mention:

    It is not safe to re-render into a container that was modified by something other than React. This worked previously in some cases but was never supported. We now emit a warning in this case. Instead you should clean up your component trees using ReactDOM.unmountComponentAtNode.

    I think we should be OK but IIRC, we do allow some legacy jQuery plugins to futz around with DOM nodes that we've created via React. I don't think they fall into this use case, but just in case we notice any funny business, I guess we should consider using ReactDOM.unmountComponentAtNode.

@@ -4,12 +4,15 @@
* Implementation of a panel that can slide up and down if jQuery
* is present on the page. If jQuery is not present, it gracefully
* degrades to a panel without animation.
*
* Note that this component requires react-transition-group@1.x
* as defined in our package.json.

This comment has been minimized.

@toolness

toolness Apr 2, 2018

Contributor

Hmm, is this really needed? My main concern is doc rot, e.g. if we eventually upgrade to react-transition-group v2.x but forget to change this line or whatever. It just seems like this is evident from the import statement a few lines down.

This comment has been minimized.

@jseppi

jseppi Apr 2, 2018

Contributor

maybe...I initially tried to use 2.x of react-transition-group but had a hard time figuring out why it wasn't working. Then I went back the docs, which said that v1.x is a drop-in replacement for react-addon-transition-group.

James Seppi and others added some commits Apr 2, 2018

James Seppi
rm use of DefinePlugin
Webpack v4's `mode` handles setting `process.env.NODE_ENV` based on the value of `mode`
so we do not need to do it manually via the `DefinePlugin`.
Merge pull request #1687 from 18F/upgrade-webpack
Upgrade babel, webpack, and node
@jseppi

This comment has been minimized.

Contributor

jseppi commented Apr 2, 2018

@toolness: good point about the various polyfills. babel-polyfill will handle Map and Set requirement. I added raf to handle the requestAnimationFrame requirement.

I do think we should re-evaulate our polyfill strategy to be a little smarter about when/what we include. Right now, we always include our polyfills in our bundle. We could, for instance, only include raf when the user is is IE < 11, via a conditional IE comment or something. I'll open a separate issue for that work.

@jseppi jseppi merged commit bd0780b into develop Apr 2, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@jseppi jseppi deleted the upgrade-react-v16 branch Apr 2, 2018

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