Fix componentDidUpdate when updating by setState on v16 #1133

koba04 · 2017-09-19T10:16:01Z

In v16, ShallowRenderer doesn't call componentDidUpdate even updating by setState.
facebook/react#10372
This PR is to fix this.

…etState in v16

koba04 · 2017-09-19T14:09:30Z

l know ShallowRenderer v16 calls cDU without a context argument but I didn't include that because I think it should be fixed separately.

ljharb · 2017-09-23T16:32:26Z

packages/enzyme-adapter-react-13/src/ReactThirteenAdapter.js

@@ -143,6 +143,7 @@ class ReactThirteenAdapter extends EnzymeAdapter {
    let isDOM = false;
    let cachedNode = null;
    return {
+      disableComponentDidUpdateOnSetState: true,


This seems like a really awkward way to extend the adapter interface.

What about this?

return { options: { disableComponentDidUpdateOnSetState: true }

I can do this without to extend the adapter interface but it requires to access the React version directly from ShallowWrapper. I think ShallowWrapper should depend on adapters only.

I wonder if the option should actually be the opposite. enableComponentDidUpdateOnSetState, making it an opt-in flag for adapters, and one that only react 16 will be using?

@lelandrichardson I think ShallowRenderer won't support componentDidUpdate in future releases so the flag is required by only versions before v16. Using enableComponentDidUpdateOnSetState means that enzyme has to set the flag for future versions of React permanently. So I used disableComponentDidUpdateOnSetState for this.
But it might make sense to use enableComponentDidUpdateOnSetState and set the flag until dropping v15 support.

that makes sense. I think there might be more use cases for adding "options" for the adapter. I think i like giving them their own namespace though: options.enableComponentDidUpdateOnSetState or something.

Also, I wonder if it's better to put this on the adapter instance itself instead of the object it returns from createRenderer? This way, we can set an empty this.options = { ... } in the EnzymeAdapter base constructor, which would mean people wouldn't have to set defaults.

Thoughts?

That makes sense! I've updated this PR.

ljharb · 2017-09-23T16:32:44Z

packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx

@@ -3403,8 +3403,7 @@ describe('shallow', () => {

    context('updating state', () => {
      // NOTE: There is a bug in react 16 shallow renderer where prevContext is not passed
-      // into componentDidUpdate. Skip this test for react 16 only. add back in if it gets fixed.


prevContext should be passed into componentDidUpdate; is react not going to fix this?

Yes. #1139 is a PR for that.
You can see the change at [Unreleased] section in CHANGELOG.md

https://github.com/facebook/react/blob/master/CHANGELOG.md

Then if that's fixed, why do we need an option in the adapter?

The PR is to support that componentDidUpdate no longer receives prevContext argument, which is not only ShallowRenderer.
This PR is to support that ShallowRenderer no longer call componentDidUpdate.
These are different fixes so I added the options each other.

Why does componentDidUpdate not receive a prevContext argument anymore? It seems like it should.

The reason seems to be the following. cc: @bvaughn

Even with that parameter present- if context isn't managed in the same way as props and state (PR #8655) then it could be inconsistent between the instance and the fiber.

Update: Chatted in meatspace with @acdlite about this. He pointed out another existing stack limitation with fiber WRT shouldComponentUpdate. (Seems like context is already flaky but fiber will make it more so.) The only safe way to use context is to pass singleton/static props (eg the way react-redux Provider passes down store) that components subscribe to for changes.

facebook/react#8631 (comment)

Wow, that seems like a pretty major change to the way context works then.

I've not found context to be flaky at all in React <= 15.

@lelandrichardson any suggestions for a better name for this part of the adapter interface?

Edit: My previous comment linked to the same response @koba04 did. Totally not helpful. Sorry. 😄 It's Monday morning.

There are pretty decent notes on facebook/react/pull/8631. The discussion/debate on how to handle prevContext in 16 stretched across 4 months. It was somewhat controversial. But I think the resolution we landed at was the best choice of those that we had. Context will need to change in the future to avoid the potential pitfalls it has always had (and still currently has).

In the meanwhile, this is still good advice:

The only safe way to use context is to pass singleton/static props...that components subscribe to for changes.

lelandrichardson · 2017-09-26T02:43:32Z

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

@@ -145,6 +145,12 @@ function nodeToHostNode(_node) {
 }

 class ReactSixteenAdapter extends EnzymeAdapter {
+  constructor() {
+    super();
+    this.options = { ...this.optins,


lelandrichardson

I'm comfortable with this. @ljharb what do you think?

ljharb · 2017-09-26T03:06:09Z

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js

@@ -145,6 +145,12 @@ function nodeToHostNode(_node) {
 }

 class ReactSixteenAdapter extends EnzymeAdapter {
+  constructor() {
+    super();
+    this.options = { ...this.options,


...this.options needs to go on its own line

ljharb · 2017-09-26T03:06:20Z

packages/enzyme-test-suite/package.json

@@ -40,4 +40,4 @@
    "react": "^15.5.0",
    "react-dom": "^15.5.0"
  }
-}
+}


all files need to have a trailing newline

ljharb · 2017-09-26T03:06:51Z

packages/enzyme/src/ShallowWrapper.js

+        const prevState = instance.state;
+        const prevContext = instance.context;
+        let shouldRender = true;
+        // dirty hack: avoid calling shouldComponentUpdate twice


why is this needed? can you elaborate in the comment?

Added a comment. Does this make sense?

ljharb

LGTM once these updates are done

ljharb · 2017-09-26T03:38:09Z

packages/enzyme/src/ShallowWrapper.js

+        ) {
+          originalShouldComponentUpdate = instance.shouldComponentUpdate;
+          instance.shouldComponentUpdate = (...args) => {
+            shouldRender = originalShouldComponentUpdate.apply(instance, args);


should this function also instance.shouldComponentUpdate = originalShouldComponentUpdate?

I wonder if this should use sinon as a dependency rather than overwriting; what about things like enumerability, setters, etc?

I wonder if this should use sinon as a dependency rather than overwriting; what about things like enumerability, setters, etc?

Yeah, I can write this like the following if I use sinon.

let shouldRender = true; let spy; if (instance.shouldComponentUpdate) { spy = sinon.spy(instance, 'shouldComponentUpdate'); } instance.setState(state, callback); if (spy) { shouldRender = spy.getCall(0).returnValue; spy.restore(); }

Also sinon seems to have a better support about enumerability so I think it's worth to consider this after releasing v3. ES2015 Proxy might be an option for that.

I'd prefer to avoid Proxy; I'm not sure it would work here anyways, and it would very tightly constrict the engines enzyme can run on.

True. To support < Node v6, sinon.spy would be nice.

A followup PR is always appreciated :-D

I'll work on it 👍

lelandrichardson · 2017-09-26T04:35:27Z

made small tweak based on @ljharb's comment. merging now in interest of time. Thanks!

koba04 · 2017-09-26T05:04:32Z

Thanks!

austinh · 2017-10-10T16:12:58Z

Has this been merged into v3.1.0? Because I cannot get componentDidUpdate to run when using setState

ljharb · 2017-10-10T16:17:47Z

Yes, it's in v3.1; please file a new issue if you have trouble still.

Shallow Renderer does not call componentDidUpdate when an update by s…

d5373a3

…etState in v16

koba04 changed the title ~~[WIP]Fix componentDidUpdate when updating by setState on v16~~ Fix componentDidUpdate when updating by setState on v16 Sep 19, 2017

ljharb added the v3 blocker label Sep 20, 2017

ljharb assigned lelandrichardson Sep 20, 2017

ljharb requested a review from lelandrichardson September 20, 2017 07:02

koba04 mentioned this pull request Sep 23, 2017

Fix componentDidUpdate no longer receives prevContext on React v16 #1139

Merged

ljharb requested changes Sep 23, 2017

View reviewed changes

lelandrichardson reviewed Sep 26, 2017

View reviewed changes

lelandrichardson approved these changes Sep 26, 2017

View reviewed changes

ljharb reviewed Sep 26, 2017

View reviewed changes

ljharb approved these changes Sep 26, 2017

View reviewed changes

ljharb reviewed Sep 26, 2017

View reviewed changes

ljharb force-pushed the fix-component-did-update-on-shallow branch from ea4b71c to d5373a3 Compare September 26, 2017 03:48

move the resetting of sCU to the overridden function

89e2354

lelandrichardson merged commit 8cf9661 into enzymejs:master Sep 26, 2017

koba04 deleted the fix-component-did-update-on-shallow branch September 26, 2017 05:04

ljharb mentioned this pull request Sep 27, 2017

Shallow renderer never call componentDidUpdate in v16. #1058

Closed

koba04 mentioned this pull request Sep 28, 2017

Implement spyMethod and use it to inspect the result of shouldComponentUpdate #1192

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix componentDidUpdate when updating by setState on v16 #1133

Fix componentDidUpdate when updating by setState on v16 #1133

koba04 commented Sep 19, 2017 •

edited

Loading

koba04 commented Sep 19, 2017

ljharb Sep 23, 2017

koba04 Sep 24, 2017

lelandrichardson Sep 26, 2017

koba04 Sep 26, 2017

lelandrichardson Sep 26, 2017

koba04 Sep 26, 2017

ljharb Sep 23, 2017

koba04 Sep 24, 2017

ljharb Sep 24, 2017

koba04 Sep 24, 2017

ljharb Sep 24, 2017

koba04 Sep 25, 2017

ljharb Sep 25, 2017

bvaughn Sep 25, 2017 •

edited

Loading

lelandrichardson Sep 26, 2017

koba04 Sep 26, 2017

lelandrichardson left a comment

ljharb Sep 26, 2017

ljharb Sep 26, 2017

ljharb Sep 26, 2017

koba04 Sep 26, 2017

ljharb left a comment

ljharb Sep 26, 2017

koba04 Sep 26, 2017

ljharb Sep 26, 2017

koba04 Sep 27, 2017

ljharb Sep 27, 2017

koba04 Sep 27, 2017

lelandrichardson commented Sep 26, 2017

koba04 commented Sep 26, 2017

austinh commented Oct 10, 2017

ljharb commented Oct 10, 2017

Fix componentDidUpdate when updating by setState on v16 #1133

Fix componentDidUpdate when updating by setState on v16 #1133

Conversation

koba04 commented Sep 19, 2017 • edited Loading

koba04 commented Sep 19, 2017

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

bvaughn Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lelandrichardson 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

ljharb 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

lelandrichardson commented Sep 26, 2017

koba04 commented Sep 26, 2017

austinh commented Oct 10, 2017

ljharb commented Oct 10, 2017

koba04 commented Sep 19, 2017 •

edited

Loading

bvaughn Sep 25, 2017 •

edited

Loading