Bugfix/403 #410

janhoogeveen · 2018-04-04T15:54:02Z

What:

Adds the react-lifecycles-compat polyfill

Why:

Otherwise you can't use Downshift with React 16.3.x and StrictMode

How:

The polyfill (official one from the React team) ensures libraries can support older React versions while adding support for newer lifecycle methods. It does this by polyfilling the Downshift component with new lifecycle methods.

Checklist:

Documentation N/A
Tests
Ready to be merged
Added myself to contributors table

I'm not familiar with Preact, but it looks like tests break when you use the polyfill in a Preact environment. I wrote an if statement to check if a component is a class component before applying the polyfill which ensures the Preact test suite passes, but the code coverage is not less than 100% and I have trouble testing that if statement. Some help with creating a test for this conditional is welcome.

… Mode

kentcdodds

We don't need the polyfill unless we're using the new lifecycle methods. Could you update this PR to use the new lifecycle methods rather than the deprecated componentWill* lifecycle methods?

kentcdodds · 2018-04-04T16:08:23Z

src/downshift.js

@@ -952,6 +954,11 @@ class Downshift extends Component {
  }
 }

+// Polyfill your component so the new lifecycles will work with older versions of React:
+if (Component.prototype && Component.prototype.isReactComponent) {


Add /* istanbul ignore else (preact) */ above this line and that'll take care of coverage 👍

Done. Code coverage is 100% now.

kentcdodds · 2018-04-04T16:09:49Z

src/downshift.js

@@ -952,6 +954,11 @@ class Downshift extends Component {
  }
 }

+// Polyfill your component so the new lifecycles will work with older versions of React:
+if (Component.prototype && Component.prototype.isReactComponent) {
+  polyfill(Downshift)


I'm pretty sure that we'll need to reassign Downshift to this for this to work properly:

Downshift = polyfill(Downshift)

Perhaps we should add a test to make sure this polyfill is applied (not certain how to do that though... maybe it's not worth it).

Just kidding. I just checked and you've got this right. Thanks! We probably still need to actually use the new lifecycle methods though...

kentcdodds · 2018-04-04T16:10:32Z

By the way, thank you so much for doing this!

janhoogeveen · 2018-04-04T16:40:04Z

From what I can see Downshift is not using any deprecated lifecycle methods! That’s the great news.

Strict Mode calls the new lifecycle methods in order to test if they work and if there are unforeseen side effects. My feeling is you don’t nees to specify them at all (since there’s no deprecated lifecycle methods at all) and that they return null or undefined by default?

kentcdodds · 2018-04-04T16:42:21Z

Oh, gotcha. Hmmm... I'm not sure I want to add the extra dependency just for this. Seems like dead weight :-/

janhoogeveen · 2018-04-04T16:42:32Z

Testing if the polyfill is applied should be doable by calling a new lifecycle method and checking if it returns an expected value. The polyfill has tests as well and since Strict Mode calls these methods it should be straightforward to copy their logic into Downshift.

janhoogeveen · 2018-04-04T16:43:37Z

Agree. It would be much better if you could exclude (sub)components in Strict Mode.

codecov-io · 2018-04-05T08:31:36Z

Codecov Report

Merging #410 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #410   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         335    337    +2     
  Branches       90     91    +1     
=====================================
+ Hits          335    337    +2

Impacted Files	Coverage Δ
src/downshift.js	`100% <100%> (ø)`	⬆️

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 9ce1e7b...5346a7d. Read the comment docs.

kentcdodds

Hmmmm... I'm a bit confused about why this is necessary and how this causes issues with StrictMode. If you look at the code in react-lifecycles-compat you'll see that it only modifies the component prototype if it's using one of the new lifecycle methods. So this code is basically dead weight. Are you certain this is necessary? Could you explain it to me?

janhoogeveen · 2018-04-05T14:47:38Z

I tried out this branch with npm link today in my own project and I still got errors. I must be doing something wrong.

I'm starting to think downshift shouldn't even give any issues with React 16.3, as it depends on React as a peer dependency right? Downshift is not built against a specific version of React as it stands?

kentcdodds · 2018-04-05T15:01:12Z

I'm starting to think downshift shouldn't even give any issues with React 16.3, as it depends on React as a peer dependency right? Downshift is not built against a specific version of React as it stands?

That is correct. It should just work out of the box with React 16.3. If you could dig a little further to figure out why you're seeing errors that would be great. From what I can tell you shouldn't be seeing any errors at all.

drobannx · 2018-04-17T03:27:00Z

I noticed a similar issue with Downshift when using React.StrictMode. I debugged some and found what I think is going on.

When turning on strict mode, react says the following:

Strict mode can’t automatically detect side effects for you, but it can help you spot them 
by making them a little more deterministic. This is done by intentionally double-invoking 
the following methods:

- Class component constructor method
- The render method
- setState updater functions (the first argument)
- The static getDerivedStateFromProps lifecycle

What is happening is the internalSetState function is doing the following:
var isStateToSetFunction = typeof stateToSet === 'function'; outside the closure that is returned.

This is problematic because later this is invoked:
stateToSet = isStateToSetFunction ? stateToSet(state) : stateToSet;

The stateToSet was originally a function and works great the first time but when invoked a subsequent time due to the use of React.StrictMode, stateToSet is now an object but isStateToSetFunction remains true due to the closure.

It looks like the stateToSet reference gets updated with the newState instead of a new reference being created.

Hope this helps! Let me know if you need me to provide any more info.

kentcdodds · 2018-04-17T03:41:56Z

That's very interesting @janhoogeveen! Thank you. I think that issue is unrelated to this PR in particular. Do you suppose you could file a new issue with everything you just said so we can come up with a solution? Suggestions welcome!

drobannx · 2018-04-17T03:51:39Z

@kentcdodds - I'm not @janhoogeveen but I can create another issue with a suggestion :)

Edit: actually, the issue (#403) that led me here is applicable but I think needs more details, I will just add my findings there.

kentcdodds · 2018-04-17T04:17:42Z

Whoops, my bad 😅

janhoogeveen · 2018-04-18T16:02:19Z

Closed this PR since I think we can all agree the first attempt was in vain. Curious to see what @drobannx comes up with!

drobannx · 2018-04-18T18:19:47Z

@janhoogeveen - I have a PR #423 with proposed changes and some more discussion

Jan Hoogeveen added 3 commits April 4, 2018 15:35

Add CLEVER°FRANKE as contributor

c2c501f

Polyfill with react-lifecycles-compat so we can use React 16.3 Strict…

febf4ea

… Mode

Adds react-lifecycle-polyfill

1be5185

janhoogeveen mentioned this pull request Apr 4, 2018

React.StrictMode issue #403

Closed

kentcdodds requested changes Apr 4, 2018

View reviewed changes

Fixes code coverage

5346a7d

kentcdodds reviewed Apr 5, 2018

View reviewed changes

janhoogeveen closed this Apr 18, 2018

janhoogeveen deleted the bugfix/403 branch April 18, 2018 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/403 #410

Bugfix/403 #410

janhoogeveen commented Apr 4, 2018 •

edited

Loading

kentcdodds left a comment

kentcdodds Apr 4, 2018

janhoogeveen Apr 5, 2018

kentcdodds Apr 4, 2018

kentcdodds Apr 4, 2018

kentcdodds commented Apr 4, 2018

janhoogeveen commented Apr 4, 2018

kentcdodds commented Apr 4, 2018

janhoogeveen commented Apr 4, 2018

janhoogeveen commented Apr 4, 2018

codecov-io commented Apr 5, 2018

kentcdodds left a comment

janhoogeveen commented Apr 5, 2018

kentcdodds commented Apr 5, 2018

drobannx commented Apr 17, 2018

kentcdodds commented Apr 17, 2018

drobannx commented Apr 17, 2018 •

edited

Loading

kentcdodds commented Apr 17, 2018

janhoogeveen commented Apr 18, 2018

drobannx commented Apr 18, 2018

Bugfix/403 #410

Bugfix/403 #410

Conversation

janhoogeveen commented Apr 4, 2018 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Apr 4, 2018

Choose a reason for hiding this comment

janhoogeveen Apr 5, 2018

Choose a reason for hiding this comment

kentcdodds Apr 4, 2018

Choose a reason for hiding this comment

kentcdodds Apr 4, 2018

Choose a reason for hiding this comment

kentcdodds commented Apr 4, 2018

janhoogeveen commented Apr 4, 2018

kentcdodds commented Apr 4, 2018

janhoogeveen commented Apr 4, 2018

janhoogeveen commented Apr 4, 2018

codecov-io commented Apr 5, 2018

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

janhoogeveen commented Apr 5, 2018

kentcdodds commented Apr 5, 2018

drobannx commented Apr 17, 2018

kentcdodds commented Apr 17, 2018

drobannx commented Apr 17, 2018 • edited Loading

kentcdodds commented Apr 17, 2018

janhoogeveen commented Apr 18, 2018

drobannx commented Apr 18, 2018

janhoogeveen commented Apr 4, 2018 •

edited

Loading

drobannx commented Apr 17, 2018 •

edited

Loading