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

.setProps() doesn't trigger componentDidUpdate lifecycle method #34

Closed
yzimet opened this issue Nov 20, 2015 · 30 comments
Closed

.setProps() doesn't trigger componentDidUpdate lifecycle method #34

yzimet opened this issue Nov 20, 2015 · 30 comments

Comments

@yzimet
Copy link

yzimet commented Nov 20, 2015

When using shallow, the .setProps() method doesn't trigger the componentDidUpdate lifecycle method. However, .setState() does trigger it. It seems like they both should (or both shouldn't).

simple example
describe('MyComponent', () => {
  let spy;

  before(function() {
    spy = sinon.spy(MyComponent.prototype, 'componentDidUpdate');
  });

  after(function() {
    spy.restore();
  });

  // fails
  it('calls componentDidUpdate when setting props', () => {
    const wrapper = shallow(<MyComponent color="yellow" />);
    wrapper.setProps({ 'color': 'green' });
    expect(spy.calledOnce).to.be.true;
  });

  // passes
  it('calls componentDidUpdate when setting state', () => {
    const wrapper = shallow(<MyComponent color="yellow" />);
    wrapper.setState({ foo: 'bar' });
    expect(spy.calledOnce).to.be.true;
  });
});
@blainekasten
Copy link
Contributor

FWIW, setProps is being deprecated.

https://github.com/facebook/react/pull/5588/files
http://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#new-deprecations-introduced-with-a-warning

@lelandrichardson
Copy link
Collaborator

@blainekasten thanks for that info. Funny - I didn't even know .setProps() existed as a real API.

The deprecation doesn't affect us in this case, since .setProps() is achieved using setState() of a parent wrapper component, anyway.

@CurtisHumphrey
Copy link
Contributor

At the moment I'm doing

const prevProp = wrapper.props()
wrapper.setProps({
  current_step: 2,
})
// forces an update call to componentDidUpdate - https://github.com/airbnb/enzyme/issues/34
wrapper.instance().componentDidUpdate(prevProp)

@anorudes
Copy link

anorudes commented May 3, 2016

+1, how correctly to solve the problem?)

@aweary
Copy link
Collaborator

aweary commented May 4, 2016

I'm betting this will be resolved by #318

@erikthedeveloper
Copy link
Contributor

I'm also performing similar hacks to what @CurtisHumphrey mentioned above.

I wonder what the preferred way of achieving this is, if not with setProps? I have yet to dig in too deep myself, but this is something I am definitely interested in contributing to. It looks like #318 is on a good track.

@blainekasten
Copy link
Contributor

#318 is now merged. Do we know if this is still a problem? Can any of the affected devs here confirm?

@bricejlin
Copy link

hi @blainekasten I'm using the latest release 2.4.1 and it still hasn't fixed the issue. I ended up having to use mount to trigger the componentDidUpdate. Would love to just use shallow instead

@koba04
Copy link
Contributor

koba04 commented Aug 30, 2016

@bricejlin
#318 is hidden in the lifecycleExperimental flag.
You can do with the flag for enabling all lifecycle methods in shallow.

shallow(<Foo />, { lifecycleExperimental: true })

@aweary
Copy link
Collaborator

aweary commented Nov 12, 2016

Closing since this should be resolved by using lifecycleExperimental flag.

@jose920405
Copy link

jose920405 commented Oct 23, 2017

In react-native lifecycleExperimental works for componentDidMount() but not for componentDidUpdate().

"name": "enzyme",
"version": "3.1.0",
"react": "16.0.0",
"react-native": "0.47.2",

#1279

@mrbinky3000

This comment has been minimized.

@ljharb

This comment has been minimized.

@mrbinky3000
Copy link

mrbinky3000 commented Sep 18, 2018

Sorry if this was off-topic. I can discuss this further offline if you want. I've been thinking about this for a while now and would like to know more about why you think this is bad. But here are some things to consider.

  1. All tests are contrived.
  2. If you properly unit test all the class methods and/or functions for a given Component, then the sum of your units should be ok. Garbage in garbage out, and vice versa.
  3. You still need integration tests like shallow and mount. For example, I still use them to test any conditional statements in a render() method. My argument is people need a fraction of the shallow and mount tests that they create today.

@ljharb
Copy link
Member

ljharb commented Sep 18, 2018

The sum of your unit tests is not equal to integration tests. You might be able to “get by” with more focused unit tests and fewer wrappers, but those aren’t better tests - those are more contrived since, unlike the wrappers, they don’t mimic production usage.

@gatispriede
Copy link

gatispriede commented Nov 9, 2018

Solved it by using

         wrapper.setProps({
            user: {
                test: {error: "test"}
            }
        });

        wrapper.find('.smth').simulate('click');

Works as a charm, setprops are set, you just need to call a function again that triggers the component o re-render, it will trigger the componentDidUpdate function

@LiangMingChen
Copy link

LiangMingChen commented Mar 8, 2019

The latest update seems to broke this again.
node_modules/enzyme/build/ShallowWrapper.js
line 552

   if (lifecycles.componentDidUpdate && typeof instance.componentDidUpdate === 'function' && (!state || (0, _Utils.shallowEqual)(state, _this3.instance().state))) {
                    instance.componentDidUpdate(prevProps, state, snapshot);
                  }

The last condition make it that only state change will trigger instance.componentUpdate,

@ljharb
Copy link
Member

ljharb commented Mar 8, 2019

@LiangMingChen could you file a new issue about this, and fill in the entire issue template? That would really help it get fixed quickly.

@fttriquet

This comment has been minimized.

@ljharb

This comment has been minimized.

@fttriquet

This comment has been minimized.

@justinmckenzie
Copy link

Sorry to bring an old thread back up, but I'm running into a similar issue detailed here, and I can't seem to find any other solutions when searching. Here's my test:

  it('should reset gridWrapperNode scrollTop position if data has changed', () => {
    const mounted = mount(<Grid {...props} />);

    const gridNode = document.createElement('div');
    const gridWrapper = document.createElement('div');

    gridWrapper.classList.add('datagrid-grid-wrapper');
    gridNode.appendChild(gridWrapper);

    mounted.instance().gridNode = gridNode;

    gridWrapper.scrollTop = 40;
    mounted.instance().lastScrollTop = 40;

    mounted.setProps({
      ...props,
      data: [{ val: 'someNewData' }],
    });

    // mounted.instance().componentDidUpdate({});

    expect(gridWrapper.scrollTop).toEqual(0);
    expect(mounted.instance().lastScrollTop).toEqual(0);
  });

When I uncomment out // mounted.instance().componentDidUpdate({});, it works as expected, however I'm expecting the componentDidUpdate method to be called with the props changing. I checked, the props are updating as they should so I was wondering if this is a known issue or if it's something I'm doing wrong?

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

@Jmac1523 try adding mounted.update().

@justinmckenzie
Copy link

justinmckenzie commented Jul 25, 2019

@ljharb I tried, unfortunately to no avail. Is the test less-effective or even ineffective with mounted.instance().componentDidUpdate({}); ?

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

@Jmac1523 can you file a new issue?

@justinmckenzie
Copy link

sure, thanks for getting back to me!

@JSONRice
Copy link

@Jmac1523 that instance call will only work on state based components. I suppose lifecycle methods only exist in those but then there is the challenge of testing the useEffect method (on update, render, etc.). IMHO there needs to be some support for this from the Enzyme team. shallow doesn't buy you much as Enzyme shallow isn't going to be tied into your lifecycle methods.

@justinmckenzie
Copy link

Err...I know, I never said I was using Hooks

@JSONRice
Copy link

JSONRice commented Nov 15, 2019

@Jmac1523 I realize that but with so many components being developed with hooks and moving away from state based it's worth noting. If you find any leads on this let me know. I'm doing some research on a state based approach now. This is something that needs attention from the Enzyme team.

@JSONRice
Copy link

JSONRice commented Nov 15, 2019

@Jmac1523 and everyone else. This solution seems to work nicely for testing state-based component lifecycle methods:

it('should call componentDidUpdate', () => {
  let node = document.createElement('div');
  let instance = ReactDOM.render(table, node);
  const spy = jest.spyOn(Table.prototype, 'componentDidUpdate');
  ReactDOM.render(modifiedTable, node);
  expect(spy).toHaveBeenCalled();
})

This is hitting that component during runtime:

image

Ultimately what I'm doing here is setting the table (JSX) on the DOM then just loading in a table with modified properties. Here's the table in Jest:

const justTheTable = (
    <Table
      data={fossilsMainDirectory}
      defaultSortType="string"
      defaultSortKey="class"
      defaultSortAsc={true}
      columnConfiguration={columns.desktop}
      tabletOverrideColumnConfiguration={columns.tablet}
      mobileOverrideColumnConfiguration={columns.mobile}
    >
      {({ filter }) => {
        return (
          <>
            <h1>Hello Spec</h1>
          </>
        );
      }}
    </Table>
  );

const table = (
    <ThemeProvider theme={theme}>
      {justTheTable}
    </ThemeProvider>
  );

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