Uplift Terra to use React 16 #1194

JakeLaCombe · 2018-02-01T18:38:21Z

Summary

The Pull Request you have all been waiting for! I was looking through the different packages, and here are the biggest non passive changes that affected us

React 16 removes [data-reactroot] from the React Element
Enzyme with react-16 does not work with rendering portals. Given our test coverage with nightwatch and wdio covered these cases, I opted just to remove the render tests.
The new datepicker opts to use popper instead of tether for it's implementation.
The focusable attribute on SVG nodes takes a string as opposed to a boolean for it's value.
For our portal usage, we can use the new api as opposed to the legacy one, as indicated by the new imports.
React 16 requires requestAnimationFrame, meaning we need to add the raf polyfill to work with legacy IE browsers.
HTML Attributes are more strict. If you had a boolean and it expects a string, it won't convert it

Deployment LInk: https://terra-core-deployed-pr-1194.herokuapp.com/

JakeLaCombe · 2018-02-01T18:38:43Z

I'll leave commentary on my code review changes so that it provides more context.

JakeLaCombe · 2018-02-01T18:40:03Z

packages/terra-alert/tests/jest/Alert.test.jsx

@@ -24,7 +24,7 @@ describe('Alert with no props', () => {

  it('throws error on missing locale prop in Base', () => {
    try {
-      shallow(<Alert />);
+      render(<Alert />);


shallow doesn't pass a context to the component anymore, so we need to do a full render to test that condition.

JakeLaCombe · 2018-02-01T18:42:00Z

packages/terra-base/src/Base.jsx

@@ -48,13 +48,21 @@ class Base extends React.Component {

  componentDidMount() {
    if (this.props.locale !== undefined) {
-      i18nLoader(this.props.locale, this.setState, this);
+      try {
+        i18nLoader(this.props.locale, this.setState, this);


For React 16, it no longer renders the component if it errors out. https://reactjs.org/blog/2017/07/26/error-handling-in-react-16.html

We can use componentDidCatch, but it gets hard with this component, as we never defined what it should do in that case. We just let it error out previously, which no longer works for us as nothing will render. We can switch to the ErrorComponent when we have established what we would like to do when a locale isn't found.

bjankord · 2018-02-01T18:48:54Z

packages/terra-arrange/tests/jest/Arrange.test.jsx

-    expect(wrapper.instance().props.fitStartAttributes).toEqual({ style: { maxWidth: '10px' } });
-    expect(wrapper.instance().props.fillAttributes).toEqual({ style: { maxWidth: '20px' } });
-    expect(wrapper.instance().props.fitEndAttributes).toEqual({ style: { maxWidth: '30px' } });
+    expect(wrapper).toMatchSnapshot();


bjankord · 2018-02-01T18:53:45Z

packages/terra-date-picker/src/_PopperContainer.scss

+  .popper-container {
+    position: absolute;
+    top: 0;
+    z-index: 2147483647;


Is the intent to set this to a value so high, z-index values on other elements wouldn't be higher?

Yeah. It's the same z-index that tether originally set so that it's widgets would display on top.

It's the same z-index

We should add an inline comment mentioning that

JakeLaCombe · 2018-02-01T19:02:57Z

packages/terra-date-time-picker/src/DateTimeUtils.js

@@ -40,7 +40,7 @@ class DateTimeUtils {
  }

  static formatMomentDateTime(momentDate, format) {
-    return momentDate && momentDate.isValid() ? momentDate.format(format) : '';
+    return momentDate && momentDate.isValid() ? momentDate.format(format) : undefined;


This change is needed to resolve the following error.

warning.js:33 Warning: Failed prop type: Invalid prop `selected` of type `string` supplied to `DatePicker`, expected `object`.

JakeLaCombe · 2018-02-01T19:05:36Z

packages/terra-modal/tests/jest/Modal.test.jsx

@@ -10,8 +10,3 @@ it('should mount an open modal', () => {
  const modal = mount(<ModalExample />);
  expect(modal).toMatchSnapshot();
 });
-
-it('should render an open modal', () => {
-  const modal = render(<ModalExample />);


For those curious, this line produced the following error.

<--- Last few GCs ---> [42935:0x102801600] 25833 ms: Mark-sweep 1405.6 (1452.6) -> 1405.6 (1452.6) MB, 1810.7 / 0.0 ms allocation failure GC in old space requested [42935:0x102801600] 27430 ms: Mark-sweep 1405.6 (1452.6) -> 1405.6 (1440.6) MB, 1596.5 / 0.0 ms last resort GC in old space requested [42935:0x102801600] 29223 ms: Mark-sweep 1405.6 (1440.6) -> 1405.6 (1440.6) MB, 1792.8 / 0.0 ms last resort GC in old space requested <--- JS stacktrace ---> ==== JS stack trace ========================================= Security context: 0x123410ba5ee1 <JSObject> 1: read [/Users/jl021230/Documents/PopulationHealth/terra/terra-core/node_modules/react-dom/cjs/react-dom-server.node.development.js:~2206] [pc=0x26d1330d7379](this=0x12341a902ef9 <ReactDOMServerRenderer map = 0x12342cf40529>,bytes=0x1234906829a9 <Number inf>) 3: renderToStaticMarkup [/Users/jl021230/Documents/PopulationHealth/terra/terra-core/node_modules/react-dom/cjs/react-dom-server.... FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory 1: node::Abort() [/Users/jl021230/.nvm/versions/node/v8.9.2/bin/node] 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/jl021230/.nvm/versions/node/v8.9.2/bin/node] 3: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [/Users/jl021230/.nvm/versions/node/v8.9.2/bin/node] 4: v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [/Users/jl021230/.nvm/versions/node/v8.9.2/bin/node] 5: v8::internal::Runtime_AllocateInTargetSpace(int, v8::internal::Object**, v8::internal::Isolate*) [/Users/jl021230/.nvm/versions/node/v8.9.2/bin/node] 6: 0x26d132f8463d 7: 0x26d1330d7379

I posted on enzymes repo, but didn't get much luck in regards to a solution.

JakeLaCombe · 2018-02-01T19:06:50Z

packages/terra-site/src/TestContainer.jsx

+
+const TestContainer = ({ children }) => (
+  <div className={styles.app} style={{ overflow: 'auto' }}>
+    <div id="wdio-test-wrapper">


This will be our base component. Not though that we still have to have individual for some tests, as they want to display in an individual component inside the wrapper.

JakeLaCombe · 2018-02-01T19:07:37Z

packages/terra-site/src/examples/embedded-content-consumer/consumers/CustomEventsConsumer.jsx

@@ -21,7 +21,6 @@ const CustomEventsConsumer = () => (
    <EmbeddedContentConsumer
      src="#/tests/embedded-content-consumer-tests/custom-events-provider"
      eventHandlers={eventHandlers}
-      fill


Did we have a reason for these props? I couldn't find any documentation on them, and I found leaving them off still resulted in passing tests, and visual regression didn't reveal any noticeable behavior.

JakeLaCombe · 2018-02-01T19:08:18Z

packages/terra-status/tests/jest/Status.test.jsx

@@ -34,15 +34,6 @@ it('should render a arrange with status and customProps', () => {
  expect(wrapper).toMatchSnapshot();
 });

-// Prop Tests
-it('should have all props including customProps set correctly', () => {


This test is covered by the test above. I would have switched this to a snapshot, but the tests on line 34 already does that for us.

JakeLaCombe · 2018-02-01T19:09:13Z

packages/terra-tabs/src/_TabMenu.jsx

@@ -76,6 +76,8 @@ class TabMenu extends React.Component {

  wrapOnClick(child) {
    return (event) => {
+      event.stopPropagation();


Essentially, when we click on a tab, the new bounding rectangle is bubbling the click up to the menu click as well, which keeps the menu bar open.

Is this something you found with the wdio screenshots?? Or in other test instances?

I found it through the wdio screenshots, but then tested it in a browser and saw the same behavior.

JakeLaCombe · 2018-02-01T19:09:57Z

packages/terra-tabs/tests/wdio/tabs-spec.js

    });
    describe('Extended', () => {
      beforeEach(() => {
        browser.url('/#/tests/tabs-tests/extended');
        browser.setViewportSize(viewport);
        browser.moveToObject('.tabContent');
      });
-      Terra.should.matchScreenshot({ viewports });
-      Terra.should.beAccessible({ viewports });
+      Terra.should.matchScreenshot({ viewports: [viewport] });


this should reduce test time. We are already looping through all the viewports at this test, so I don't see a large benefit in having our screenshot comparison match at all of them for every single viewport.

JakeLaCombe · 2018-02-01T19:11:30Z

packages/terra-time-input/tests/nightwatch/TimeInputTests.jsx

@@ -14,8 +14,6 @@ const TimeInputTests = () => (
      <li><Link to="/tests/time-input-tests/twelve-hour-filled-morning">TimeInput - Twelve Hour Default Time Morning</Link></li>
      <li><Link to="/tests/time-input-tests/twelve-hour-filled-evening">TimeInput - Twelve Hour Default Time Evening</Link></li>
      <li><Link to="/tests/time-input-tests/twelve-hour-mobile">TimeInput - Twelve Hour Mobile</Link></li>


I had to combine all the tests into the same component due to race conditions. I noticed that sometimes, componentWillMount would render before componentWillUnmount of the previous component, meaning sometimes the fake mobile mode would be disabled. Thus, I kept it at one component on the same page to ensure this doesn't happen.

emilyrohrbough · 2018-02-01T19:13:38Z

packages/terra-button/tests/wdio/button-spec.js

@@ -1,14 +1,4 @@
 /* global browser, Terra, before */
-const shouldTheme = (customProperties, selector) => {


Nice. Glad this is finally gone :)

I'll get rid of the terra-form-radio on another pull request

Actually that one is different and needs to stay for now. The toolkit implementation will apply each variable one at at time and take a screenshot for each change. To get the correct theming screenshots for form-radio, multiple variables need to change at one time due to the default being browser naive.

terra-bot · 2018-02-01T19:13:41Z

	Warnings
⚠️	❗ Big PR. Consider breaking this into smaller PRs if applicaple

Generated by 🚫 dangerJS

emilyrohrbough · 2018-02-01T19:16:25Z

packages/terra-icon/src/IconBase.jsx

@@ -37,7 +37,7 @@ const propTypes = {
  /**
   * Focusable attribute. IE 10/11 are focusable without this attribute.
   */
-  focusable: PropTypes.bool,
+  focusable: PropTypes.string,


Why this change?

focusable is actually a string node for SVGs (https://allyjs.io/tutorials/focusing-in-svg.html) and react was throwing prop type errors when we gave it a boolean instead of a string.

emilyrohrbough · 2018-02-01T19:16:38Z

packages/terra-icon/CHANGELOG.md

@@ -3,6 +3,8 @@ Changelog

 Unreleased
 ----------
+### Changed
+* Update to React 16


This needs update to reflect the prop type change.

emilyrohrbough · 2018-02-01T19:17:22Z

packages/terra-legacy-theme/CHANGELOG.md

@@ -3,6 +3,7 @@ Changelog

 Unreleased
 ----------
+* Update to React 16


emilyrohrbough · 2018-02-01T19:17:50Z

packages/terra-mixins/CHANGELOG.md

@@ -3,6 +3,7 @@ Changelog

 Unreleased
 ----------
+* Update to React 16


JakeLaCombe · 2018-02-01T19:20:19Z

terra-form-radio has it's own implementation of wdio, but to get this pull request moving, I left off that change to another future pull request. It will produce tons of snapshots, and to better test visual regression, I want to leave the snapshots as unchanged as possible (which they are right now 😄 )

emilyrohrbough · 2018-02-01T19:53:38Z

packages/terra-tabs/tests/wdio/tabs-spec.js

@@ -15,26 +15,26 @@ describe('Tabs - Responsive', () => {
        browser.setViewportSize(viewport);
        browser.moveToObject('.tabContent');
      });
-      Terra.should.matchScreenshot({ viewports });
-      Terra.should.beAccessible({ viewports });
+      Terra.should.matchScreenshot({ viewports: [viewport] });


You should be able reduce this to Terra.should.matchScreenshot();

Thanks! Removed it, and it works great!

799ad0e

mmalaker · 2018-02-08T22:30:30Z

Found issue in RtL orientation, when you click on the calendar button for a date picker within a modal, the calendar does not display. I'm testing the rest of the site in RTL to see if there are any other bugs.

mmalaker · 2018-02-09T15:17:56Z

Rechecked the RTL date picker issue and that is now resolved. No other issues found. Functional verification completed.

noahbenham · 2018-02-10T00:28:33Z

I know this is closed but we were running into similar heap memory issues with our project. If this continues happening for terra-core and wasn't isolated to that line you removed, here's what we did @JakeLaCombe.

Root cause was including babel-polyfill in our jestSetup.js. Removing that took our heap memory usage from ~1.5GB to ~125MB. It doesn't look like that's true for this repo though.

We diagnosed our issue with node --expose-gc ./node_modules/.bin/jest --runInBand --logHeapUsage. That might be useful in case this continues happening and wasn't caused by the line you removed.

* Uplift Terra to use React 16 * Fix DateTime Utils * Revert date-time-picker test change * Fix code of testing screenshots * Fix changelogs * Document need for large z-index * Fix Popper Settings for Datepicker * Fix wdio styles * Clean Up Tests * Clean Up Lint

Uplift Terra to use React 16

2afeeff

bjankord temporarily deployed to terra-core-deployed-pr-1194 February 1, 2018 18:38 Inactive

JakeLaCombe commented Feb 1, 2018

View reviewed changes

Merge branch 'master' into Terra-React-16-Uplift

0117e8c

bjankord temporarily deployed to terra-core-deployed-pr-1194 February 1, 2018 18:44 Inactive

bjankord reviewed Feb 1, 2018

View reviewed changes

Fix DateTime Utils

408a142

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 1, 2018 18:54 Inactive

Revert date-time-picker test change

204ef1d

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 1, 2018 19:00 Inactive

JakeLaCombe commented Feb 1, 2018

View reviewed changes

emilyrohrbough reviewed Feb 1, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into Terra-React-16-Uplift

1bdadaf

Fix Popper Settings for Datepicker

0d28c5a

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 03:43 Inactive

Merge branch 'master' into Terra-React-16-Uplift

0df3156

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 04:29 Inactive

bjankord approved these changes Feb 9, 2018

View reviewed changes

mmalaker removed the Functional Review Required label Feb 9, 2018

Merge branch 'master' into Terra-React-16-Uplift

fa99088

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 15:19 Inactive

bjankord added the do not merge label Feb 9, 2018

Merge branch 'master' into Terra-React-16-Uplift

09a40e3

bjankord temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 18:02 Inactive

Merge branch 'master' into Terra-React-16-Uplift

cb2f5db

bjankord temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 19:29 Inactive

bjankord removed the do not merge label Feb 9, 2018

yuderekyu approved these changes Feb 9, 2018

View reviewed changes

Fix wdio styles

6c22340

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 21:22 Inactive

Clean Up Tests

b5d70a5

JakeLaCombe temporarily deployed to terra-core-deployed-pr-1194 February 9, 2018 21:35 Inactive

Clean Up Lint

300983c

JakeLaCombe deployed to terra-core-deployed-pr-1194 February 9, 2018 21:37 View deployment

JakeLaCombe merged commit 01273b8 into master Feb 9, 2018

JakeLaCombe deleted the Terra-React-16-Uplift branch February 9, 2018 22:05

bjankord mentioned this pull request Feb 9, 2018

React 16 upgrade #658

Closed

4 tasks

benbcai mentioned this pull request Feb 12, 2018

React 16 uplift cerner/terra-clinical#272

Merged

cody-dot-js mentioned this pull request Feb 24, 2018

CWELL-15943 Uplift to React v16 cerner/terra-consumer#116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uplift Terra to use React 16 #1194

Uplift Terra to use React 16 #1194

JakeLaCombe commented Feb 1, 2018 •

edited

Loading

JakeLaCombe commented Feb 1, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

bjankord Feb 1, 2018

bjankord Feb 1, 2018

JakeLaCombe Feb 1, 2018

emilyrohrbough Feb 1, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

emilyrohrbough Feb 1, 2018

JakeLaCombe Feb 2, 2018

JakeLaCombe Feb 1, 2018

JakeLaCombe Feb 1, 2018

emilyrohrbough Feb 1, 2018

JakeLaCombe Feb 1, 2018

emilyrohrbough Feb 1, 2018

terra-bot commented Feb 1, 2018 •

edited

Loading

emilyrohrbough Feb 1, 2018

JakeLaCombe Feb 1, 2018

emilyrohrbough Feb 1, 2018 •

edited

Loading

emilyrohrbough Feb 1, 2018

emilyrohrbough Feb 1, 2018

JakeLaCombe commented Feb 1, 2018 •

edited

Loading

emilyrohrbough Feb 1, 2018

JakeLaCombe Feb 1, 2018

mmalaker commented Feb 8, 2018

mmalaker commented Feb 9, 2018

noahbenham commented Feb 10, 2018

		@@ -1,14 +1,4 @@
		/* global browser, Terra, before */
		const shouldTheme = (customProperties, selector) => {

Uplift Terra to use React 16 #1194

Uplift Terra to use React 16 #1194

Conversation

JakeLaCombe commented Feb 1, 2018 • edited Loading

Summary

JakeLaCombe commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terra-bot commented Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeLaCombe commented Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalaker commented Feb 8, 2018

mmalaker commented Feb 9, 2018

noahbenham commented Feb 10, 2018

JakeLaCombe commented Feb 1, 2018 •

edited

Loading

terra-bot commented Feb 1, 2018 •

edited

Loading

emilyrohrbough Feb 1, 2018 •

edited

Loading

JakeLaCombe commented Feb 1, 2018 •

edited

Loading