Fix double render on selector change

[salvoravida]

This pr just set the current value before store subscribe/resubscribe
This prevent double rendering when using selectorFamily in child nodes, controlled by parent props.

Issue CodeSandbox:
https://codesandbox.io/s/recoil-legacy-selector-change-double-render-issue-fhbxk

Fixed CodeSandbox:
https://codesandbox.io/s/recoil-legacy-selector-change-double-render-fixed-feub8

[facebook-github-bot]

@drarmstr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[drarmstr]

Thank you so much for continuing to improve this optimization!

Could you generate a unit test that helps confirm the situation where this change eliminates an extra render? You can see an example for such test with "Updating with same value doesn't rerender" test in https://github.com/facebookexperimental/Recoil/blob/master/src/recoil_values/__tests__/Recoil_atom-test.js

Note that I updated the unit tests from importing #820 so compensate for that fix in removing the extra render when the GK is set which you should hopefully see when that merges.

[drarmstr]

@salvoravida - #820 has now been merged. Notice in the update that it was tweaked to be behind the GK feature flag check and the unit tests were updated. We'll see if that causes any issues with nightly testing.. But, from those examples hopefully you can see how the unit testing works. Could you update this PR with an example test to help clarify the situations it affects and catch potential regressions?

[facebook-github-bot]

@salvoravida has updated the pull request. You must reimport the pull request before landing.

[facebook-github-bot]

@salvoravida has updated the pull request. You must reimport the pull request before landing.

[salvoravida]

Hi @drarmstr, i have just rebased the pr on last master commit, moreover i added the test for this case.
it should also fix another test with one less re-render that is a similar case: selector unmounting

[drarmstr]

@salvoravida - Ah, yes, excellent! Thank you for the test! I'll try to review this next week then. With #952 landing now we can start to open up this optimization for our internal testing again.

[facebook-github-bot]

@drarmstr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[drarmstr]

@salvoravida - I had a chance to go over the test and it looks good. However, I'm still not clear on what change the fix is supposed to do. It looks like the test works both with and without the code changes in Recoil_Hooks.js.

[salvoravida]

@salvoravida - I had a chance to go over the test and it looks good. However, I'm still not clear on what change the fix is supposed to do. It looks like the test works both with and without the code changes in Recoil_Hooks.js.

Hi @drarmstr (as first comment)
This pr just set the current value before store subscribe/resubscribe
This prevent double rendering when using selectorFamily in child nodes, controlled by parent props.

Issue CodeSandbox:
codesandbox.io/s/recoil-legacy-selector-change-double-render-issue-fhbxk

Fixed CodeSandbox:
codesandbox.io/s/recoil-legacy-selector-change-double-render-fixed-feub8

about tests:

With this pr(my mod on Recoil_Hooks)
Test Suites: 34 passed, 34 total
Tests: 2 skipped, 811 passed, 813 total

Without:
Test Suites: 1 failed, 33 passed, 34 total
Tests: 2 failed, 2 skipped, 809 passed, 813 total

● Components unsubscribe from atoms when rendered without using them [recoil_suppress_rerender_in_callback]

expect(jest.fn()).toHaveBeenCalledTimes(expected)

Expected number of calls: 3
Received number of calls: 4

  825 |     act(() => toggleSwitch());
  826 |     expect(container.textContent).toEqual('0');
> 827 |     expect(Component).toHaveBeenCalledTimes(baseCalls + 3);

● Components re-render only one time if selectorFamily changed [recoil_suppress_rerender_in_callback]

expect(jest.fn()).toHaveBeenCalledTimes(expected)

Expected number of calls: 2
Received number of calls: 3

  1758 | 
  1759 |     expect(container.textContent).toEqual('1');
> 1760 |     expect(Component).toHaveBeenCalledTimes(baseCalls + 2);

Are you sure you have tested the OSS version? may have some problems with flags?

[drarmstr]

Let me give it another run through with the legacy vs concurrent mode enabled, maybe that was the issue. On Apr 26, 2021, at 00:55, Salvatore Ravidà ***@***.***> wrote:  @salvoravida<https://github.com/salvoravida> - I had a chance to go over the test and it looks good. However, I'm still not clear on what change the fix is supposed to do. It looks like the test works both with and without the code changes in Recoil_Hooks.js. Hi @drarmstr<https://github.com/drarmstr> (as first comment) This pr just set the current value before store subscribe/resubscribe This prevent double rendering when using selectorFamily in child nodes, controlled by parent props. Issue CodeSandbox: codesandbox.io/s/recoil-legacy-selector-change-double-render-issue-fhbxk Fixed CodeSandbox: codesandbox.io/s/recoil-legacy-selector-change-double-render-fixed-feub8 about tests: With this pr(my mod on Recoil_Hooks) Test Suites: 34 passed, 34 total Tests: 2 skipped, 811 passed, 813 total Without: Test Suites: 1 failed, 33 passed, 34 total Tests: 2 failed, 2 skipped, 809 passed, 813 total ● Components unsubscribe from atoms when rendered without using them [recoil_suppress_rerender_in_callback] expect(jest.fn()).toHaveBeenCalledTimes(expected) Expected number of calls: 3 Received number of calls: 4 825 | act(() => toggleSwitch()); 826 | expect(container.textContent).toEqual('0');

827 | expect(Component).toHaveBeenCalledTimes(baseCalls + 3);

● Components re-render only one time if selectorFamily changed [recoil_suppress_rerender_in_callback] expect(jest.fn()).toHaveBeenCalledTimes(expected) Expected number of calls: 2 Received number of calls: 3 1758 | 1759 | expect(container.textContent).toEqual('1');

1760 | expect(Component).toHaveBeenCalledTimes(baseCalls + 2);

Are you sure you have tested the OSS version? may have some problems with flags? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#825 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABDVJ2BBEDL5FOSKGJQZ3JLTKUL6ZANCNFSM4V4XFL4Q>.

[salvoravida]

@drarmstr note that Suspence pr is not merged on my branch.

[salvoravida]

@drarmstr note that Suspence pr is not merged on my branch.

Hi @drarmstr i tested this PR over the Suspense one. it seems the suspense one has the same effect on the test.
Anyway, there is one extra optimization in one test with this pr:
'Components unsubscribe from atoms when rendered without using them',

it allows one render less on legacy mode, and component unsubscribe.

[drarmstr]

Ah, good; glad the additional Suspense fix explains the discrepancy. Ok, I'll test again with that test as soon as I have a chance. Thanks!

[drarmstr]

Finally got around to validating and landing this optimization while working on the latest rendering modes. Thanks again @salvoravida.