Upgrade to React==16.4.1 & Enzyme==3.3.0 #5359

mistercrunch · 2018-07-06T15:50:54Z

The upgrade was pretty smooth except for a cryptic message coming
out of react-select around running multiple copies of React. It turns
out the common bundle had React and was conflicting with explore and
dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x
outside of the wasted resources.

Running 16.4 should bring in all sorts of perf improvements and features
we've all been waiting for.

https://reactjs.org/blog/2017/09/26/react-v16.0.html

BLOCKERs

react-bootstrap-datetimepicker appears to be deprecated and not be React 16 ready (using React.propTypes), FYI @betodealmeida said he'd look into this specific issue
in some cases the enzyme pattern of calling wrapper.instance().method() and then assert(wrapper.find()) seems to not work in [at least] some cases. That results in 4-5 unit tests that are still failing

@williaster @graceguo-supercat curious to hear your thoughts on this as this is a bit related to #5370

williaster · 2018-08-08T06:06:55Z

this is really exciting! what's the plan from here?

williaster · 2018-08-15T21:00:03Z

@mistercrunch any update here?

mistercrunch · 2018-09-04T19:12:50Z

@williaster, had a conversation with @betodealmeida about this and he said he's willing to tackle the react-bootstrap-datetimepicker blocker, and I can commit to push through the finish line once this is done.

betodealmeida · 2018-09-05T03:33:33Z

#5819

williaster · 2018-09-05T21:58:46Z

awesome 🎉 super exciting!

The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16

mistercrunch · 2018-09-10T18:23:50Z

@williaster this is ready for review, I had to disable a few tests to allow moving forward with this

codecov-io · 2018-09-10T18:51:47Z

Codecov Report

Merging #5359 into master will increase coverage by 14.1%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5359      +/-   ##
==========================================
+ Coverage    63.6%   77.71%   +14.1%     
==========================================
  Files         374       46     -328     
  Lines       23321     9301   -14020     
  Branches     2608        0    -2608     
==========================================
- Hits        14833     7228    -7605     
+ Misses       8475     2073    -6402     
+ Partials       13        0      -13

Impacted Files	Coverage Δ
superset/db_engine_specs.py	`53.14% <0%> (-2.05%)`	⬇️
superset/utils.py	`88.12% <0%> (-0.9%)`	⬇️
superset/connectors/sqla/models.py	`80.37% <0%> (-0.76%)`	⬇️
superset/models/core.py	`84.6% <0%> (-0.5%)`	⬇️
superset/views/core.py	`74.03% <0%> (-0.3%)`	⬇️
...sets/src/explore/components/ExploreChartHeader.jsx
superset/assets/src/components/Checkbox.jsx
...perset/assets/src/dashboard/util/getEmptyLayout.js
...ts/src/dashboard/util/getComponentWidthFromDrop.js
...erset/assets/src/SqlLab/components/QuerySearch.jsx
... and 316 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e35bfba...44b9b1a. Read the comment docs.

williaster

Sweet! Can we just fix the tests instead of disabling them? I would view that as part of the work needed for the upgrade. Esp since it seems like it's just 2.

For the specific ones that you disabled, I think you may need to call wrapper.update() before the assertion as described in this section of the migration guide.

williaster · 2018-09-10T19:45:16Z

superset/assets/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx

@@ -3,7 +3,7 @@ import React from 'react';
 import sinon from 'sinon';
 import { expect } from 'chai';
 import { describe, it } from 'mocha';
-import { shallow } from 'enzyme';
+import { mount, shallow } from 'enzyme';


do you really need mount or do you know why it is required now but not previously? It's good to avoid that when possible. wondering if calling wrapper.update() would fix (tho the docs say that applies to just mounted components)

williaster · 2018-09-10T19:45:46Z

superset/assets/spec/javascripts/explore/components/DateFilterControl_spec.jsx

-    setTimeout(() => {
-      expect(wrapper.find(Button)).to.have.length(3);
-    }, 10);
+    expect(popover.find(Button)).to.have.length(1);


why is this 1 but the title says 3?

williaster

we strongly believe that these tests should be fixed, not removed.

especially since we're trying to move toward a more stable / less-buggy codebase, we don't think it's worth that cost to get to react 16 faster.

williaster · 2018-09-10T19:48:50Z

superset/assets/spec/javascripts/explore/components/DateFilterControl_spec.jsx

  });
-  it('loads the right state', () => {


can we add these back?

mistercrunch · 2018-09-10T20:03:01Z

@williaster thanks for pointing to the enzyme upgrade docs, it helped me understand what was going on. I think this is good to go now.

williaster

Thanks for taking the time to fix the tests! This is an exciting update! 🎉

* Bumping react==16.4.1 & enzyme==3.3.0 The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16 * Trying to deprecate react-bootstrap-datetime * Moving forward * Reintroducing tests (cherry picked from commit 73db918) (cherry picked from commit 5128ae3)

* Bumping react==16.4.1 & enzyme==3.3.0 The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16 * Trying to deprecate react-bootstrap-datetime * Moving forward * Reintroducing tests (cherry picked from commit 73db918)

* Bumping react==16.4.1 & enzyme==3.3.0 The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16 * Trying to deprecate react-bootstrap-datetime * Moving forward * Reintroducing tests (cherry picked from commit 73db918) (cherry picked from commit 5128ae3)

* Bumping react==16.4.1 & enzyme==3.3.0 The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16 * Trying to deprecate react-bootstrap-datetime * Moving forward * Reintroducing tests

* Bumping react==16.4.1 & enzyme==3.3.0 The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16 * Trying to deprecate react-bootstrap-datetime * Moving forward * Reintroducing tests (cherry picked from commit 73db918) (cherry picked from commit 5128ae3)

* Bumping react==16.4.1 & enzyme==3.3.0 The upgrade was pretty smooth except for a cryptic message coming out of react-select around running multiple copies of React. It turns out the `common` bundle had React and was conflicting with explore and dashboard apps, only in 16.x. This somehow wasn't a problem in 15.x outside of the wasted resources. Running 16.4 should bring in all sorts of perf improvements and features we've all been waiting for. https://reactjs.org/blog/2017/09/26/react-v16.0.html TODO: react-bootstrap-datetimepicker isn't compatible with React 16 * Trying to deprecate react-bootstrap-datetime * Moving forward * Reintroducing tests

mistercrunch force-pushed the react_16 branch 3 times, most recently from 7a26372 to 6cc2b3c Compare July 18, 2018 20:39

mistercrunch force-pushed the react_16 branch from 6cc2b3c to 20bd2cd Compare September 4, 2018 16:48

mistercrunch added 3 commits September 10, 2018 10:44

Trying to deprecate react-bootstrap-datetime

e110548

Moving forward

75a5c76

mistercrunch force-pushed the react_16 branch from 20bd2cd to 75a5c76 Compare September 10, 2018 18:19

mistercrunch changed the title ~~[WiP] bump react==16.4.1 & enzyme==3.3.0~~ Upgrade to React==16.4.1 & Enzyme==3.3.0 Sep 10, 2018

williaster reviewed Sep 10, 2018

View reviewed changes

williaster requested changes Sep 10, 2018

View reviewed changes

williaster reviewed Sep 10, 2018

View reviewed changes

Reintroducing tests

44b9b1a

mistercrunch force-pushed the react_16 branch from 8e653ff to 44b9b1a Compare September 10, 2018 20:00

williaster approved these changes Sep 10, 2018

View reviewed changes

mistercrunch merged commit 73db918 into apache:master Sep 10, 2018

mistercrunch deleted the react_16 branch September 10, 2018 21:48

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to React==16.4.1 & Enzyme==3.3.0 #5359

Upgrade to React==16.4.1 & Enzyme==3.3.0 #5359

mistercrunch commented Jul 6, 2018 •

edited

Loading

williaster commented Aug 8, 2018

williaster commented Aug 15, 2018

mistercrunch commented Sep 4, 2018

betodealmeida commented Sep 5, 2018

williaster commented Sep 5, 2018

mistercrunch commented Sep 10, 2018

codecov-io commented Sep 10, 2018 •

edited

Loading

williaster left a comment

williaster Sep 10, 2018

williaster Sep 10, 2018

williaster left a comment

williaster Sep 10, 2018

mistercrunch commented Sep 10, 2018

williaster left a comment

Upgrade to React==16.4.1 & Enzyme==3.3.0 #5359

Upgrade to React==16.4.1 & Enzyme==3.3.0 #5359

Conversation

mistercrunch commented Jul 6, 2018 • edited Loading

BLOCKERs

williaster commented Aug 8, 2018

williaster commented Aug 15, 2018

mistercrunch commented Sep 4, 2018

betodealmeida commented Sep 5, 2018

williaster commented Sep 5, 2018

mistercrunch commented Sep 10, 2018

codecov-io commented Sep 10, 2018 • edited Loading

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

williaster Sep 10, 2018

Choose a reason for hiding this comment

williaster Sep 10, 2018

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

williaster Sep 10, 2018

Choose a reason for hiding this comment

mistercrunch commented Sep 10, 2018

williaster left a comment

Choose a reason for hiding this comment

mistercrunch commented Jul 6, 2018 •

edited

Loading

codecov-io commented Sep 10, 2018 •

edited

Loading