Upgrade to react 16 #191

tobilen · 2017-09-30T17:35:10Z

This kind of snowballed.

the old version of storybook doesnt work with react16. it was throwing exceptions before, so i just upgraded it to the most recent version.
This meant webpack had to be upgraded, so i've done that as well. The stories are all looking good so far.

The big problem with react 16 is enzyme. I imported the adapter, but chai-enzyme doesnt play nice with enzyme 3 yet.
There is an open PR which solves most of the issues. the others i circumvented by refactoring the assertions a bit. until its merged, you need to build chai-enzyme yourself:

cd node_modules/chai-enzyme
yarn && yarn build

The setProps() call doesnt seem to be working the same way in enzyme 3, which made the mountAttached + attachedWrapper.setProps(...) logic in the tests break. i worked around this by remounting for every test, but this of course makes the old/new position comparisons not work.

=================================
Update:
Refactored testing component so it passes its state to flip-move instead of its props. This means we can use setState(...) instead of setProps(...) to trigger a transition.
I had to make an empty setState(...) call after initial mounting because ...well, setState updates in enzyme 3 still appear to be a bit wonky.
I also removed chai-enzyme assertions for the time being.

joshwcomeau · 2017-09-30T17:57:12Z

Oh wow! Thanks so much for this, whole project is modernized :D

If I understand correctly:

chai-enzyme is incompatible with Enzyme 3, and Enzyme 3 is needed for React 16
There's an open PR on chai-enzyme (Make compatible with enzyme v3 enzymejs/chai-enzyme#196?) which addresses this, but in the meantime the tests can pass if you build it yourself, since you've specified a fork right now.
The tests fail now because we can't use setProps anymore. Looking at the code, it seems like you're just mounting with the value, which means that the tests are failing (I presume because there's no longer a transition between two states, it just mounts at the final state?)

For those first two points, I wonder if chai-enzyme is worth the trouble. It's certainly a convenience, but I feel like regular Chai is good enough (as I see you've done, by just using to.equal instead of to.have.id).

For the third point... I initially thought this was an issue of state carrying between tests, but I remembered that @Hypnosphi's great work meant that we are already mounting for every describe. I ran the tests locally and it is a bit tough to tell what's going on, but yeah I think the issue is that we need some way to update the props after the component has mounted; everything other than appear animations works on transitions between props.

So, i think we need to figure out what the deal is with setProps. Do you know more about how it changed / why it doesn't work anymore?

tobilen · 2017-09-30T18:09:56Z

To your first point, correct.
To your second point, not exactly. Using the fork means we get past this line, but, like you've observed yourself, there dont seem to be many chai enzyme assertions that actually work. Removing them entirely might be more feasible.

Third point seems to be the biggest issue, i agree. I've read through the enzyme migration guide a couple of times now, but i dont see anything that explains why setProps throws. The only viable workaround i see atm is defining a wrapper component that passes its state as props, and then just calling setState on the wrapper.

setState doesnt work like it did before, as i learned during the upgrade of our codebase at work yesterday. It doesnt throw, but it also doesnt always trigger rerenders on children (enzymejs/enzyme#1153 ), but maybe we can mitigate that by calling wrapper.update(). I'll try that tomorrow

tobilen · 2017-10-01T14:12:07Z

We're looking good now. I'd still like to add a story (and test) for passing fragments and string to flip-move though.

tobilen · 2017-10-01T14:42:49Z

test for proper fragment rendering ( 9022b85 ) and string rendering (a4673cf) has been added, i'd say we're good to go now.

joshwcomeau

Wooo!!

Thanks so much for this, and sorry it took me a few days to get to it (In Mountainview this week, working for my job on-site, so it's been hectic).

I had a really small nit, but I'm happy to quickly do that myself in a subsequent commit.

Will test this locally tomorrow just to make sure nothing funky happens, and will publish a release ASAP!

joshwcomeau · 2017-10-04T19:22:06Z

.flowconfig

@@ -3,6 +3,8 @@
 .*/node_modules/editions/.*
 .*/node_modules/flow-coverage-report/.*
 .*/node_modules/preact/.*
+.*/node_modules/@storybook/.*
+.*/node_modules/radium/.*


Guessing modern versions of Storybook use Radium?

Not directly, but via react-treebeard. We use glamorous for our own components

Oh cool! Wasn't aware you were a contributor to Storybook - thanks for your work there :)

joshwcomeau · 2017-10-04T19:23:04Z

package.json

-    "babel": "6.1.18",
-    "babel-cli": "6.2.0",
-    "babel-core": "6.2.1",
+    "@storybook/react": "^3.2.8",


Nit: can we omit the ^?

I don't really think this even matters anymore, with Yarn/NPM5. But, my inner OCD wants it to be consistent, haha.

You probably also want to upgrade it to 3.2.12, there has been some critical bugfixes

joshwcomeau · 2017-10-04T19:24:45Z

webpack.config.production.js

@@ -8,6 +8,7 @@ config.plugins = [
  new webpack.DefinePlugin({
    'process.env.NODE_ENV': JSON.stringify('production'),
  }),
+  new webpack.optimize.ModuleConcatenationPlugin(),


Oooh, neat! Keen to see the effect this has on bundle size.

Hypnosphi

LGTM

Hypnosphi · 2017-10-04T21:58:41Z

package.json

@@ -46,29 +46,29 @@
    "web-animations"
  ],
  "peerDependencies": {
-    "react": "0.13.x || 0.14.x || 15.x.x",
-    "react-dom": "0.13.x || 0.14.x || 15.x.x"
+    "react": ">0.13.x <16.x.x",


http://jubianchi.github.io/semver-check/

Good catch 👍

Looks like >=0.13.x <=16.x.x works

joshwcomeau · 2017-10-05T16:11:22Z

Hm, so I pulled this locally, tried running storybook, and I get the following errors:

I tried removing node_modules and reinstalling, but it still seems like there's some React version discrepancy.

Have you encountered this? Any tips for how to get past it?

tobilen · 2017-10-05T18:57:35Z

@joshwcomeau try upgrading to the latest version of yarn i guess storybookjs/storybook#1868 (comment)

tobilen · 2017-10-14T18:45:22Z

@Hypnosphi can you update your review?

joshwcomeau · 2017-10-15T12:11:41Z

Woooo!!! Psyched to have this ready :D can't thank you enough for your work on this!!

joshwcomeau · 2017-10-15T12:23:41Z

Released in v2.9.17! 🎊

(Also, sorry for not debugging the yarn issue more thoroughly myself; the issue was indeed using 0.x yarn, upgrading fixed it!)

tobilen added 6 commits September 30, 2017 17:51

Upgrade react to version 16, update to enzyme 3

1367edf

Use enzyme adapter, start fixing tests

e66261f

Upgrade storybook, babel and webpack dependencies

bc9f6a7

Upgrade babel-preset-flow, fix testrunner to work with webpack 3

465fe66

Accept react 0.13 to 16 as peer dependency

67a280f

Remove setProps calls, since they dont seem to be working right

31916e0

tobilen added the help wanted label Sep 30, 2017

tobilen added 5 commits October 1, 2017 15:59

Refactor test component to use state and setState instead of setProps

b5bb104

Fix linting

214e081

Remove chai-enzyme

a3960fe

Remove unused import

5952e41

Remove flow typechecking from storybook until its upgraded to flow 0.54+

a13a89b

tobilen removed the help wanted label Oct 1, 2017

tobilen added 4 commits October 1, 2017 16:15

Require node_js version > 5

01ac104

Ignore flow typechecks for radium depdendency

878bf30

Fix webpack config to match new api

ee2f614

Add test for react16 fragment support

9022b85

Extended fragment test to also render a string component

a4673cf

tobilen requested a review from Hypnosphi October 1, 2017 14:48

tobilen assigned joshwcomeau and unassigned joshwcomeau Oct 1, 2017

tobilen requested a review from joshwcomeau October 1, 2017 14:48

tobilen mentioned this pull request Oct 1, 2017

Add support for React 16 #190

Closed

joshwcomeau approved these changes Oct 4, 2017

View reviewed changes

Hypnosphi approved these changes Oct 4, 2017

View reviewed changes

Hypnosphi requested changes Oct 4, 2017

View reviewed changes

tobilen added 4 commits October 5, 2017 20:37

Include react 0.13.x and 16.x.x in peer dependency version range

4963f88

Fix prop type and make sure we dont pass style props to flipMove

784a04c

Upgrade to storybook 3.2.12

f096d29

Individually check if items are falsy, return them if they are

d3304dc

disable linting error for prop filtering

96779a7

Hypnosphi approved these changes Oct 14, 2017

View reviewed changes

joshwcomeau merged commit 932437d into master Oct 15, 2017

joshwcomeau deleted the upgrade-react-16 branch October 15, 2017 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to react 16 #191

Upgrade to react 16 #191

tobilen commented Sep 30, 2017 •

edited

Loading

joshwcomeau commented Sep 30, 2017

tobilen commented Sep 30, 2017 •

edited

Loading

tobilen commented Oct 1, 2017

tobilen commented Oct 1, 2017 •

edited

Loading

joshwcomeau left a comment

joshwcomeau Oct 4, 2017

Hypnosphi Oct 4, 2017

joshwcomeau Oct 5, 2017

joshwcomeau Oct 4, 2017

Hypnosphi Oct 4, 2017

joshwcomeau Oct 4, 2017

Hypnosphi left a comment

Hypnosphi Oct 4, 2017 •

edited

Loading

joshwcomeau Oct 5, 2017

joshwcomeau Oct 5, 2017 •

edited

Loading

tobilen Oct 5, 2017

joshwcomeau commented Oct 5, 2017

tobilen commented Oct 5, 2017 •

edited

Loading

tobilen commented Oct 14, 2017

joshwcomeau commented Oct 15, 2017

joshwcomeau commented Oct 15, 2017

Upgrade to react 16 #191

Upgrade to react 16 #191

Conversation

tobilen commented Sep 30, 2017 • edited Loading

joshwcomeau commented Sep 30, 2017

tobilen commented Sep 30, 2017 • edited Loading

tobilen commented Oct 1, 2017

tobilen commented Oct 1, 2017 • edited Loading

joshwcomeau 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

Hypnosphi left a comment

Choose a reason for hiding this comment

Hypnosphi Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwcomeau Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwcomeau commented Oct 5, 2017

tobilen commented Oct 5, 2017 • edited Loading

tobilen commented Oct 14, 2017

joshwcomeau commented Oct 15, 2017

joshwcomeau commented Oct 15, 2017

tobilen commented Sep 30, 2017 •

edited

Loading

tobilen commented Sep 30, 2017 •

edited

Loading

tobilen commented Oct 1, 2017 •

edited

Loading

Hypnosphi Oct 4, 2017 •

edited

Loading

joshwcomeau Oct 5, 2017 •

edited

Loading

tobilen commented Oct 5, 2017 •

edited

Loading