React 16 and Enzyme 3 migration #1885

adidahiya · 2017-12-11T22:28:54Z

Fixes #1051
Fixes #144
Fixes #866 (for real)
Fixes #1860
Fixes #1680

Changes in this PR

Upgrade react and react-dom to v16
- Load es6-shim in all tests to ensure compatibility
- Fix prop validation tests that broke due to React failing faster on thrown JS Errors
Replace react-addons-css-transition-group with react-transition-group
Replace pure-render-decorator usage with extends React.PureComponent
- Split AbstractComponent into impure AbstractComponent and pure AbstractPureComponent classes
Upgrade enzyme to v3 and install enzyme-adapter-react-16
- Fix various tests that broke due to Enzyme API changes
<Portal>: Replace ReactDOM._unstableRenderSubtreeToContainer with ReactDOM.createPortal

adidahiya

make sure to check out the lockfile

adidahiya · 2017-12-11T22:29:52Z

packages/core/src/components/overlay/overlayTransition.tsx

+import { CSSTransition } from "react-transition-group";
+import { IOverlayProps } from "./overlay";
+
+export function OverlayTransition<


this is no longer used; will remove

adidahiya · 2017-12-11T22:29:58Z

packages/core/src/components/overlay/overlay.tsx

-                transitionLeaveTimeout={transitionDuration}
-                transitionName={transitionName}
-            >
+            <TransitionGroup appear={true}>


see https://github.com/reactjs/react-transition-group/blob/master/Migration.md

giladgray · 2017-12-11T22:51:32Z

packages/core/src/components/button/buttonGroup.tsx

@@ -38,8 +37,7 @@ export interface IButtonGroupProps extends IProps, React.HTMLProps<HTMLDivElemen

 // this component is simple enough that tests would be purely tautological.
 /* istanbul ignore next */
-@PureRender
-export class ButtonGroup extends React.Component<IButtonGroupProps, {}> {
+export class ButtonGroup extends React.PureComponent<IButtonGroupProps, {}> {


we should be able to drop these empty {} state types, as the React types now provide a default: interface Component<P = {}, S = {}>.

though it's fine to not do this now, or ever.

giladgray · 2017-12-11T22:52:23Z

packages/core/src/components/icon/icon.tsx

@@ -31,8 +30,7 @@ export interface IIconProps extends IIntentProps, IProps {
    iconSize?: 16 | 20 | "inherit";
 }

-@PureRender
-export class Icon extends React.Component<IIconProps & React.HTMLAttributes<HTMLSpanElement>, never> {
+export class Icon extends React.PureComponent<IIconProps & React.HTMLAttributes<HTMLSpanElement>, never> {


remove never, or make it {} like all the others

giladgray · 2017-12-11T22:55:31Z

packages/core/src/components/overlay/overlay.tsx

-                    onMouseDown={this.handleBackdropMouseDown}
-                    tabIndex={this.props.canOutsideClickClose ? 0 : null}
-                />
+                <CSSTransition classNames={transitionName} timeout={transitionDuration}>


classNames 😱
should we do this??

that's their API. are you asking if we should do this for our own props? I don't think there's anything wrong with the current names, so would prefer to keep them

giladgray · 2017-12-11T22:56:32Z

packages/core/src/components/portal/portal.tsx

+    constructor(props: IPortalProps, context: IPortalContext) {
+        super(props, context);
+        this.targetElement = document.createElement("div");
+        this.targetElement.classList.add(Classes.PORTAL);


why move to constructor instead of didMount?

it happens sooner this way. also this is how the API example was set up

oh also I need it to be there by the time render() is called, which is not true of componentDidMount

blueprint-bot · 2017-12-13T02:33:53Z

Address follow up comments on #1898

Preview: documentation | landing | table

blueprint-bot · 2017-12-13T02:35:51Z

Remove extraneous core karma test config

Preview: documentation | landing | table

adidahiya · 2017-12-13T06:47:39Z

Some regressions I noticed (which can be addressed in follow-up PRs):

ESC key to close Dialogs doesn't work until you hit TAB to move focus into the Dialog element
When opening a <Select>, the input field is not focussed immediately (likely related to the above issue)

adidahiya · 2017-12-13T19:36:19Z

Filed #1901 to address those later ^

themadcreator

Questions. Otherwise good to go

themadcreator · 2017-12-13T22:04:27Z

packages/core/karma.conf.js

            "src/accessibility/*",
+            "src/common/abstractComponent*",
+            // deprecated components
+            "src/components/popover/*",


are these actually deprecated if they're being replaced by labs components?

we'll need to remember to remove this line when @cmslewis's Popover2 -> Popover change goes in.

themadcreator · 2017-12-13T22:09:00Z

packages/core/src/components/portal/portal.tsx

        );
    }

+    public componentDidMount() {


it looks like componentDidMount is maintained, but componentDidUpdate was removed which used an unstable method. Is this functionality still captured?

yeah so now that we're actually returning something from render instead of null all the time, the React tree works as expected. there's a unit test for the context-preserving functionality (it("respects blueprintPortalClassName on context")

themadcreator · 2017-12-13T22:12:06Z

packages/labs/test/index.ts

@@ -2,4 +2,11 @@
 * Copyright 2017 Palantir Technologies, Inc. All rights reserved.
 */

+import "es6-shim";


wasn't this supposed to be removed?

(though it's just test code code, so... yolo)

no, you explicitly need to bring it in to support React 16. We're definitely not removing es6-shim from the page runtime; we are pushing the responsibility down to consumers of the library (e.g. the test environment, table-dev-app, docs-app, our other apps, etc.)

themadcreator · 2017-12-13T22:14:33Z

packages/table/src/table.tsx

@@ -492,7 +492,7 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
    // time. it serves as a signal that we can switch to batch rendering.
    private didCompletelyMount = false;

-    public constructor(props: ITableProps, context?: any) {
+    public constructor(props: ITableProps & { children: React.ReactNode }, context?: any) {


ITableProps already has a definition for children, shouldn't these match?

yeah good catch. will fix

themadcreator · 2017-12-13T22:14:54Z

packages/table/src/table.tsx

@@ -664,7 +664,8 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
        );
    }

-    public componentWillReceiveProps(nextProps: ITableProps) {
+    public componentWillReceiveProps(nextProps: ITableProps & { children: React.ReactNode }) {


blueprint-bot · 2017-12-13T22:43:19Z

Remove extra props interface in table impl

Preview: documentation | landing | table

cmslewis

Requesting changes for the test:karma:debug thing in particular.

cmslewis · 2017-12-13T22:15:39Z

packages/core/src/components/portal/portal.tsx

    public componentWillUnmount() {
-        ReactDOM.unmountComponentAtNode(this.targetElement);


Why don't we need this anymore?

React cleans up itself when you actually return things in render

cmslewis · 2017-12-13T22:17:17Z

packages/core/src/components/tabs2/tab2.tsx

@@ -39,8 +38,7 @@ export interface ITab2Props extends IProps {
    title?: string | JSX.Element;
 }

-@PureRender
-export class Tab2 extends React.Component<ITab2Props, {}> {
+export class Tab2 extends React.PureComponent<ITab2Props, {}> {


@giladgray FYI: this edit. (You moved these files in #1900.)

cmslewis · 2017-12-13T22:21:37Z

packages/core/test/collapsible-list/collapsibleListTests.tsx

@@ -22,7 +22,7 @@ import {
 describe("<CollapsibleList>", () => {
    it("adds className to itself", () => {
        const list = renderCollapsibleList(3, { className: "winner" });
-        assert.lengthOf(list.find(".winner"), 1);
+        assert.isNotEmpty(list.find(".winner"));


Hmm, this check isn't equivalent. Was this suffering from Enzyme 3's .hostNodes() issue (enzymejs/enzyme#1174)? Seems like the fix for too many elements here is:

assert.lengthOf(list.find(".winner").hostNodes(), 1);

Yeah but is that really a more meaningful / useful test? IMO this is another tautology and I would rather have a auto-generated test suite for spreading props / classNames rather than including one of these test cases at the start of every component test suite

ok I can switch to hostNodes

cmslewis · 2017-12-13T22:29:09Z

packages/core/test/text/textTests.tsx

@@ -15,7 +15,7 @@ describe("<Text>", () => {
        const textContent = "textContent";
        const className = "bp-test-class";
        const wrapper = mount(<Text className={className}>{textContent}</Text>);
-        const element = wrapper.find(`.${className}`);
+        const element = wrapper.find(`div.${className}`);


wrapper.find(`.${className}`).hostNodes()

is clearer

cmslewis · 2017-12-13T22:29:42Z

packages/core/test/toast/toasterTests.ts

@@ -130,10 +128,11 @@ describe("Toaster", () => {

        it("focuses on newly created toast", done => {
            toaster.show({ message: "focus on me" });
+            // small explicity timeout reduces flakiness of these tests


explicitly

cmslewis · 2017-12-13T22:35:51Z

packages/labs/package.json

@@ -24,6 +24,7 @@
    "test": "npm-run-all -s test:pre -p test:karma",
    "test:pre": "tsc -p ./test",
    "test:karma": "karma start",
+    "test:karma:debug": "karma start --browsers=Chrome --single-run=false --reporters=mocha --debug",


cmslewis · 2017-12-13T22:36:13Z

packages/select/package.json

@@ -24,26 +24,28 @@
    "test": "npm-run-all -s compile:cjs test:pre -p test:karma test:iso",
    "test:pre": "tsc -p ./test",
    "test:karma": "karma start",
+    "test:karma:debug": "karma start --browsers=Chrome --single-run=false --reporters=mocha --debug",


cmslewis · 2017-12-13T22:36:37Z

packages/select/test/suggestTests.tsx

@@ -73,7 +73,7 @@ describe("Suggest", () => {

        it("scrolls active item into view when popover opens", () => {
            const wrapper = suggest();
-            const queryList = (wrapper.getNode() as any).queryList; // private ref
+            const queryList = ((wrapper.instance() as Suggest<Film>) as any).queryList; // private ref


what does double-casting do for us?

just helps clarify intent to developers

cmslewis · 2017-12-13T22:36:44Z

packages/table/package.json

@@ -24,27 +24,29 @@
    "test": "npm-run-all -s compile:cjs test:pre -p test:karma test:iso",
    "test:pre": "tsc -p ./test",
    "test:karma": "karma start",
+    "test:karma:debug": "karma start --browsers=Chrome --single-run=false --reporters=mocha --debug",


cmslewis · 2017-12-13T22:38:23Z

packages/table/src/table.tsx

@@ -492,7 +492,7 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
    // time. it serves as a signal that we can switch to batch rendering.
    private didCompletelyMount = false;

-    public constructor(props: ITableProps, context?: any) {
+    public constructor(props: ITableProps & { children: React.ReactNode }, context?: any) {


Worth making a new props interface in core/.../props.ts? This is used in a few places.

e.g.

export interface IChildrenProps { children: React.ReactNode; }

no, I removed it

blueprint-bot · 2017-12-13T23:04:06Z

Address CR feedback on tests

Preview: documentation | landing | table

addressed

hysoftwareeng · 2017-12-15T18:52:34Z

Is this going to be in the next release (after 1.35.0)? I currently still see an unmet peer dependency when installing core 1.35.0 for react-addons-css-transition-group

giladgray · 2017-12-15T19:09:06Z

@hysoftwareeng this will be in our upcoming 2.0 release, but the installation instructions do not change: react is still a peerDependency so you must explicitly install it yourself in your application.

skovy · 2018-01-17T01:32:51Z

When will a release be cut for 2.0? It's not possible to upgrade to React 16 if blueprintjs is a dependency. Thanks!

adidahiya · 2018-01-17T19:21:07Z

It's not possible to upgrade to React 16 if blueprintjs is a dependency.

What are the specific errors you run into? React 16 is supported as a peer dependency: #1660

rmendelovits · 2018-01-17T21:24:02Z

I have the following error when trying to upgrade to React 16:


npm WARN pure-render-decorator@1.2.1 requires a peer of react@^15.0.0 || ^0.14.0
 but none is installed. You must install peer dependencies yourself.
npm WARN react-addons-css-transition-group@15.6.2 requires a peer of react@^15.4
.2 but none is installed. You must install peer dependencies yourself.

But where does pure-render-decorator/react-addons-css-transition-group come from?

the pure-render-decorator is a requirement for these two packages.
@blueprintjs/core
@blueprintjs/table

and I get this warning when I try to move to react-transition-group

npm WARN @blueprintjs/core@1.35.1 requires a peer of react-addons-css-transition
-group@^15.0.1 || ^0.14 but none is installed. You must install peer dependencie
s yourself.

adidahiya · 2018-01-17T21:33:22Z

Those warnings can be safely ignored. See #866 (comment).

Anyway, I just released the first beta of blueprint 2.0.0, you can try it out for react 16 support. Release notes coming soon.

rmendelovits · 2018-01-18T01:28:44Z

Thanks

skovy · 2018-01-18T17:04:28Z

@adidahiya not sure I follow. react-addons-css-transition-group requires react@^15.4.2, and we have react@16.2.0. Does that mean react-addons-css-transition-group would then require us to have two versions of React in the bundle?

When I upgrade to React 16.2.0 and run yarn check, I'm recieve these errors:

error "react-addons-css-transition-group#react@^15.4.2" doesn't satisfy found match of "react@16.2.0"
error "@blueprintjs/core#pure-render-decorator#react@^15.0.0 \|\| ^0.14.0" doesn't satisfy found match of "react@16.2.0"
error "@blueprintjs/datetime#react-day-picker#react@~0.13.x \|\| ~0.14.x \|\| ^15.0.0" doesn't satisfy found match of "react@16.2.0"

adidahiya · 2018-01-18T20:24:53Z

@skovy Since these are peer dependencies, no, you don't need to end up with two versions of React in your bundle. Warnings aside, does this cause a problem at runtime in your application when you try to use React 16.2.0?

Also, I suggest upgrading react-day-picker to a version that allows React 16 as a peer dependency: https://github.com/gpbl/react-day-picker/blob/3156e3de1664caa15cf12cfd0fdd96350874e8c5/package.json#L55

skovy · 2018-01-21T20:30:35Z

@adidahiya got it, thanks for the insight and I appreciate the help! 👍

skovy · 2018-01-30T05:26:25Z

@adidahiya I opened a PR to upgrade react-day-picker, but ran into linting problems. Would appreciate any insights on #2055.

adidahiya added 3 commits December 11, 2017 13:31

Bump to react 16, regen lockfile

817d141

pure-render-decorator -> React.PureComponent

4478ac8

Migrate to react-transition-group

c5a98e4

adidahiya requested review from cmslewis, giladgray and jkillian December 11, 2017 22:28

adidahiya commented Dec 11, 2017

View reviewed changes

adidahiya added 3 commits December 11, 2017 14:46

Migrate to ReactDOM.createPortal

73d7a8e

Merge remote-tracking branch 'origin/master' into ad/react-16

3df77f1

Fix Table component shouldComponentUpdate warnings

49f8069

giladgray reviewed Dec 11, 2017

View reviewed changes

adidahiya added 19 commits December 11, 2017 15:00

Restore wrapper div for Portal component

c2074d5

Upgrade enzyme, install enzyme-adapter-react-16

ab2c99b

Configure isomorphic tests with enzyme adapter

cee34d5

Skip Portal isomorphic tests

9efdfbf

Load es6-shim in all tests

f2ec4e6

Fix NumericInput prop validation tests

d1c94a7

Add TestErrorBoundary component

c7fa963

Create @blueprintjs/test-commons package

a1cc25a

Fix DatePicker prop validation tests

391fadb

Fix more prop validation tests

4158d2e

Fix more prop validation tests

6d90e49

Merge remote-tracking branch 'origin/master' into ad/react-16

ecfa923

Merge remote-tracking branch 'origin/master' into ad/react-16

307297b

Fix up test config for latest Enzyme; load CSS files

02cbae9

Move tabs2 tests to their own folder

3395389

Skip more tests

ce4c03f

[CollapsibleList] fix unit tests

53d894d

Add test:karma:debug NPM scripts to libraries

583507e

Merge remote-tracking branch 'origin/master' into ad/react-16

2f5a61b

adidahiya requested review from themadcreator and removed request for jkillian December 13, 2017 06:56

adidahiya mentioned this pull request Dec 13, 2017

[2.0] Fix autofocus issues in overlays #1901

Closed

themadcreator approved these changes Dec 13, 2017

View reviewed changes

Remove extra props interface in table impl

5dbc249

cmslewis previously requested changes Dec 13, 2017

View reviewed changes

adidahiya added 2 commits December 13, 2017 14:54

Remove --browsers from test:karma:debug

aa47e76

Address CR feedback on tests

71988dd

adidahiya merged commit 0376104 into master Dec 13, 2017

adidahiya deleted the ad/react-16 branch December 13, 2017 23:07

giladgray added the Type: breaking change label Feb 9, 2018

giladgray mentioned this pull request Feb 9, 2018

React 16 support #866

Closed

3 tasks

		public componentWillUnmount() {
		ReactDOM.unmountComponentAtNode(this.targetElement);

React 16 and Enzyme 3 migration #1885

React 16 and Enzyme 3 migration #1885

Conversation

adidahiya commented Dec 11, 2017 • edited Loading

Changes in this PR

adidahiya left a 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

blueprint-bot commented Dec 13, 2017

Address follow up comments on #1898

blueprint-bot commented Dec 13, 2017

Remove extraneous core karma test config

adidahiya commented Dec 13, 2017

adidahiya commented Dec 13, 2017

themadcreator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

themadcreator Dec 13, 2017 • edited Loading

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

blueprint-bot commented Dec 13, 2017

Remove extra props interface in table impl

cmslewis left a 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

blueprint-bot commented Dec 13, 2017

Address CR feedback on tests

hysoftwareeng commented Dec 15, 2017 • edited Loading

giladgray commented Dec 15, 2017

skovy commented Jan 17, 2018

adidahiya commented Jan 17, 2018

rmendelovits commented Jan 17, 2018 • edited Loading

adidahiya commented Jan 17, 2018

rmendelovits commented Jan 18, 2018

skovy commented Jan 18, 2018

adidahiya commented Jan 18, 2018

skovy commented Jan 21, 2018

skovy commented Jan 30, 2018

adidahiya commented Dec 11, 2017 •

edited

Loading

adidahiya Dec 13, 2017 •

edited

Loading

themadcreator Dec 13, 2017 •

edited

Loading

hysoftwareeng commented Dec 15, 2017 •

edited

Loading

rmendelovits commented Jan 17, 2018 •

edited

Loading