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

Data: Refactor withSelect to use getDerivedStateFromProps #7431

Merged
merged 4 commits into from Jun 21, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Jun 21, 2018

Related: #7395

This pull request seeks to refactor the higher-order component returned by withSelect to simplify its generating of merge props using static getDerivedStateFromProps. This is to avoid calling setState during componentDidUpdate, which itself occurs after a render and would incur a second render, thus introducing a performance overhead for a component which is used broadly through the application. With these changes, it should still be the case that the component is rendered once on either a props change or store change.

Testing instructions:

Ensure unit tests pass:

npm run test-unit packages/data/src/test/index.js

@gziolo gziolo added this to the 3.1 milestone Jun 21, 2018

@youknowriad

Great refactoring as always 👍

Testing: Update tests to verify function call times
Removes case "ensures component is still mounted before setting state" which was made redundant with addition of "should run selections on parents before its children"
@aduth

This comment has been minimized.

Member

aduth commented Jun 21, 2018

Updated test cases to ensure expected number of render / mapSelectToProps / mapDispatchToProps. Verified that as suspected, these same tests fail in master:

  ● withSelect › should rerun selection on props changes

    expect(jest.fn()).toHaveBeenCalledTimes(2)
    
    Expected mock function to have been called two times, but it was called three times.

      527 | 		expect( wrapper.childAt( 0 ).text() ).toBe( '10' );
      528 | 		expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 );
    > 529 | 		expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
      530 | 	} );
      531 | 
      532 | 	it( 'should render if props have changed but not state', () => {
      
      at Object.<anonymous> (packages/data/src/test/index.js:529:31)

  ● withSelect › omits props which are not returned on subsequent mappings

    expect(jest.fn()).toHaveBeenCalledTimes(2)
    
    Expected mock function to have been called two times, but it was called three times.

      607 | 
      608 | 		expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 );
    > 609 | 		expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
      610 | 		expect( wrapper.childAt( 0 ).props() ).toEqual( { bar: 'OK', propName: 'bar' } );
      611 | 	} );
      612 | 
      
      at Object.<anonymous> (packages/data/src/test/index.js:609:31)

  ● withSelect › allows undefined return from mapSelectToProps

    expect(jest.fn()).toHaveBeenCalledTimes(2)
    
    Expected mock function to have been called two times, but it was called three times.

      640 | 
      641 | 		expect( mapSelectToProps ).toHaveBeenCalledTimes( 2 );
    > 642 | 		expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
      643 | 		expect( wrapper.childAt( 0 ).text() ).toBe( 'OK' );
      644 | 
      645 | 		wrapper.setProps( { pass: false } );
      
      at Object.<anonymous> (packages/data/src/test/index.js:642:31)
@aduth

This comment has been minimized.

Member

aduth commented Jun 21, 2018

Yikes, I got bit by running an older version of React locally. Thankfully Travis caught them.

Seems related to changes in getDerivedStateFromProps in the 16.4.0 release to call when state changes, which I thought might be an issue, but was not observing only because I was running the older version.

https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops

Will investigate a fix.

@aduth

This comment has been minimized.

Member

aduth commented Jun 21, 2018

Will investigate a fix.

In fact, the fix was an original implementation I had which was failing with the older version of React I had running locally, and works exactly as expected with 16.4.0+.

See 1b935b6.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Jun 21, 2018

Yikes, I got bit by running an older version of React locally. Thankfully Travis caught them.

Mmm, I was also using the old React version when I did all these refactorings 🤔

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 21, 2018

I merged 16.4 yesterday... :)

// to the component, including state changes.
//
// See: https://reactjs.org/blog/2018/05/23/react-v-16-4.html#bugfix-for-getderivedstatefromprops
this.setState( () => ( {} ) );

This comment has been minimized.

@gziolo

gziolo Jun 21, 2018

Member

Interesting way to trigger getDerivedStateFromProps 👍

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 21, 2018

Looks great, very happy to see the number of improvements introduced to the test suite 💯

@aduth aduth merged commit 2c2189c into master Jun 21, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +53.18% compared to 5ea93cd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/refactor-with-select branch Jun 21, 2018

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