Skip to content

Fix componentDidUpdate when updating by setState on v16 #1133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 26, 2017
Merged

Fix componentDidUpdate when updating by setState on v16 #1133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d5373a3
Shallow Renderer does not call componentDidUpdate when an update by s…
koba04 Sep 19, 2017
89e2354
move the resetting of sCU to the overridden function
lelandrichardson Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,13 @@ function nodeToHostNode(_node) {
}

class ReactSixteenAdapter extends EnzymeAdapter {
constructor() {
super();
this.options = {
...this.options,
enableComponentDidUpdateOnSetState: true,
};
}
createMountRenderer(options) {
assertDomAvailable('mount');
const domNode = options.attachTo || global.document.createElement('div');
Expand Down
1 change: 1 addition & 0 deletions packages/enzyme-test-suite/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,4 @@
"react-dom": "^15.5.0"
}
}

3 changes: 1 addition & 2 deletions packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3441,8 +3441,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.
Copy link
Member

@ljharb ljharb Sep 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@koba04 koba04 Sep 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb Sep 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@koba04 koba04 Sep 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ljharb ljharb Sep 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@koba04 koba04 Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@ljharb ljharb Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@bvaughn bvaughn Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

itIf(!REACT16, 'should call shouldComponentUpdate, componentWillUpdate and componentDidUpdate', () => {
it('should call shouldComponentUpdate, componentWillUpdate and componentDidUpdate', () => {
const spy = sinon.spy();

class Foo extends React.Component {
Expand Down
3 changes: 3 additions & 0 deletions packages/enzyme/src/EnzymeAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ function unimplementedError(methodName, classname) {
}

class EnzymeAdapter {
constructor() {
this.options = {};
}
// Provided a bag of options, return an `EnzymeRenderer`. Some options can be implementation
// specific, like `attach` etc. for React, but not part of this interface explicitly.
// eslint-disable-next-line class-methods-use-this, no-unused-vars
Expand Down
35 changes: 34 additions & 1 deletion packages/enzyme/src/ShallowWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,40 @@ class ShallowWrapper {
}
this.single('setState', () => {
withSetStateAllowed(() => {
this.instance().setState(state, callback);
const adapter = getAdapter(this[OPTIONS]);
const instance = this.instance();
const prevProps = instance.props;
const prevState = instance.state;
const prevContext = instance.context;
let shouldRender = true;
// This is a dirty hack but it requires to know the result of shouldComponentUpdate.
// When shouldComponentUpdate returns false we shouldn't call componentDidUpdate.
// shouldComponentUpdate is called in `instance.setState`
// so we replace shouldComponentUpdate to know the result and restore it later.
let originalShouldComponentUpdate;
if (
this[OPTIONS].lifecycleExperimental &&
adapter.options.enableComponentDidUpdateOnSetState &&
instance &&
typeof instance.shouldComponentUpdate === 'function'
) {
originalShouldComponentUpdate = instance.shouldComponentUpdate;
instance.shouldComponentUpdate = (...args) => {
shouldRender = originalShouldComponentUpdate.apply(instance, args);
Copy link
Member

@ljharb ljharb Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@koba04 koba04 Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ljharb ljharb Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@koba04 koba04 Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ljharb ljharb Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A followup PR is always appreciated :-D

Copy link
Contributor Author

@koba04 koba04 Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll work on it 👍

instance.shouldComponentUpdate = originalShouldComponentUpdate;
return shouldRender;
};
}
instance.setState(state, callback);
if (
shouldRender &&
this[OPTIONS].lifecycleExperimental &&
adapter.options.enableComponentDidUpdateOnSetState &&
instance &&
typeof instance.componentDidUpdate === 'function'
) {
instance.componentDidUpdate(prevProps, prevState, prevContext);
}
this.update();
});
});
Expand Down