Skip to content
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.

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

Calling setProps on shallow wrapper results in componentDidUpdate being called twice #1862

Closed
jpmoral opened this issue Oct 11, 2018 · 11 comments

Comments

@jpmoral
Copy link

jpmoral commented Oct 11, 2018

Describe the bug
Calling setProps on a shallow wrapper results in componentDidUpdate being called twice (instead of once).

To Reproduce
Steps to reproduce the behavior:
Run the test:

import React from 'react';


export default class Dummy extends React.Component {
    componentWillReceiveProps(nextProps) {
        this.setState({someState: nextProps.myProp});
    }

    componentDidUpdate() {
        this.foo();
    }

    foo() {}

    render() {
        return (
            <div>
                <div>{`myProp: ${this.props.myProp}`}</div>
            </div>
        );
    }
}
import React from 'react';
import {shallow} from 'enzyme';

import Dummy from 'dummy.js';


describe('<Dummy/>', () => {
    it('works', () => {
        const props = {};
        const wrapper = shallow(<Dummy {...props} />);
        wrapper.instance().foo = jest.fn();
        wrapper.setProps({myProp: 'Prop Value'});
        expect(wrapper.instance().foo).toHaveBeenCalledTimes(1);
    });
});

Expected behavior
componentDidUpdate should only be called once.

Desktop (please complete the following information):

  • OS: macOS Mojave
  • Browser n/a
  • Version 3.7.0

Smartphone (please complete the following information):
n/a

@jpmoral jpmoral changed the title Calling setProps on wrapperresults in componentDidUpdate being called twice Calling setProps on wrapper results in componentDidUpdate being called twice Oct 11, 2018
@ljharb
Copy link
Member

ljharb commented Oct 16, 2018

(wrapper.instance().foo = jest.fn(); isn't doing what you think it is; you need to spy on Dummy.prototype.foo before the initial render)

It seems like the issue is that it's calling cWRP, queueing up a setState, rerendering with the props only, and calling cDU, and then rerendering with the new state, and then calling cDU again.

I presume that React itself is batching up the updates so that this is working as expected - I don't believe we have proper batching support for React 16 just yet.

(cc @koba04)

@ljharb
Copy link
Member

ljharb commented Oct 16, 2018

What happens when you use mount?

@koba04
Copy link
Contributor

koba04 commented Oct 16, 2018

@ljharb
Yeah, React doesn't support batched updates on ShallowRendering from v16.

facebook/react#10355

So currently, it's a no-op.

https://github.com/airbnb/enzyme/blob/master/packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js#L327-L330

If we support batching updates on shallow to fix this, we have to implement ourselves.

@ljharb
Copy link
Member

ljharb commented Oct 16, 2018

Although that's a lot more work, I think it's probably worth it so that things are consistent between React and enzyme.

@jpmoral jpmoral changed the title Calling setProps on wrapper results in componentDidUpdate being called twice Calling setProps on shallow wrapper results in componentDidUpdate being called twice Oct 16, 2018
@jpmoral
Copy link
Author

jpmoral commented Oct 16, 2018

@ljharb thanks, mount works. I've edited the title and body to explicitly mention shallow.

@ghost
Copy link

ghost commented Dec 18, 2018

I'll go ahead and look into this issue if a fix hasn't already been applied.

@ghost
Copy link

ghost commented Dec 20, 2018

@ljharb Added the above test case, but in terms of the fix, has there been much discussion on adding support for batched updates with the ReactSixteenThreeAdapter?

I've been looking at @koba04's implementation of unstable_batchedUpdates

koba04/react@ca78c92,

but would you have any suggestions for moving forward?

@ljharb
Copy link
Member

ljharb commented Dec 20, 2018

@peanutenthusiast not much, but I'd love to add it (ideally separate from bug fixes)

@ghost
Copy link

ghost commented Feb 6, 2019

Was out of country for a month or so, but I've come up with a fix for the above test case -- @ljharb are there any other test cases that should be covered? I can send a PR tonight.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2019

Not that i know of; PR away!

@ghost ghost mentioned this issue Feb 6, 2019
@jpmoral
Copy link
Author

jpmoral commented Feb 12, 2019

Awesome, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants