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

Fix rerender when location.search changes #178

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Conversation

wuzzeb
Copy link
Contributor

@wuzzeb wuzzeb commented Feb 26, 2021

18a3a38 removed the search from the pathname returned from useLocation, but that caused a bug because useState has an optimization that it can avoid a re-render if the state has not changed. Since only the path and not the search was set in the state, React avoids a re-render when only the search changes.

https://reactjs.org/docs/hooks-reference.html#bailing-out-of-a-dispatch

Unfortunately, the test suite didn't pick that up because I suspect there is a difference between the react-testing-renderer and the real browser renderer. I have been trying and logging various re-renders in real React and react-testing-renderer. For whatever reason in the test suite, which uses react-testing-renderer, React does re-render the first time useState is called with the duplicate value, which I suspect is something to do with fibers. This difference caused the test suite to pass even though the real react skips the first update.

Eventually the react-testing-renderer does also stop re-rendering, so it is still possible to include in the test suite. The update
to "/foo?goodbye=world" is the one that fails in the test suite and does not cause a re-render.

To fix, I just include both the path and search in the state so that when the state is updated, it is always re-rendered. There is the check using the ref to prevent re-rendering, so by the time update is called, we know we need a re-render.


All this is orthogonal to the question of exposing the search values directly in the API somewhere. For myself, while it would be nice to have the search params available in the API somewhere, I am just fine parsing them myself. All I need is for the re-render to occur when it changes.

18a3a38
removed the search from the pathname returned
from useLocation, but that caused a bug because
useState has an optimzation that it can avoid a
re-render if the state has not changed.  Since
only the path and not the search was set in the
state, React avoids a re-render when only the
search changes.

https://reactjs.org/docs/hooks-reference.html#bailing-out-of-a-dispatch

Unfortunately, the test suite didn't pick that up
because I suspect there is a difference between
the react-testing-renderer and the real browser
renderer. I have been trying and logging various
re-renders in real React and react-testing-renderer.
For whatever reason in the test suite,
which uses react-testing-renderer, React does
re-render the first time useState is called with
the duplicate value, which I suspect is something
to do with fibers.  This difference caused the
test suite to pass even though the real react
skips the first update.

Eventually the react-testing-renderer does also
stop re-rendering, so it is still possible to
include in the test suite.  The update
to "/foo?goodbye=world" is the one that fails
in the test suite and does not cause a re-render.

To fix, I just include both the path and search
in the state so that when the state is updated,
it is always re-rendered.  There is the check
using the ref to prevent re-rendering, so by the time
update is called, we know we need a re-render.
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #178 (8774d36) into master (a7f0970) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #178   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          249       250    +1     
  Branches        50        50           
=========================================
+ Hits           249       250    +1     
Impacted Files Coverage Δ
use-location.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7f0970...8774d36. Read the comment docs.

Copy link
Owner

@molefrog molefrog left a comment

Choose a reason for hiding this comment

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

Interesting... So you mean React won't trigger an extra rerender if we update the state with the existing value? The fix looks good, nice work!

@molefrog molefrog merged commit da46ebc into molefrog:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants