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(v2): keep DocSearch state on remounts #3297

Merged
merged 2 commits into from
Aug 17, 2020

Conversation

francoischalifour
Copy link
Contributor

Motivation

The Docusaurus navbar remounts at a certain screen resize. This creates new variable references for objects and functions passed to the DocSearchModal component, resulting in a rerender of DocSearch that then loses the previous state (because we listen to these reference changes in our component).

This PR keeps references of DocSearch options by using useMemo and useRef.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Before

docusaurus-ds3-before

After

docusaurus-ds2-fix-after

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 17, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Aug 17, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 9f503cd

https://deploy-preview-3297--docusaurus-2.netlify.app

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Hey

What just wondering, why using refs instead of useMemo/useCallback? Do you need stronger guarantee or something?

searchClient.addAlgoliaAgent('docusaurus', siteMetadata.docusaurusVersion);

return searchClient;
}).current;
Copy link
Collaborator

Choose a reason for hiding this comment

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

oooh this is a funny pattern I've never seen before :)

@francoischalifour
Copy link
Contributor Author

@slorber history and withBaseUrl's references change at every render so I cannot use useMemo and useCallback here. Do you have a solution to avoid useRef here?

(I was able to use useCallback for transformSearchClient.)

@slorber
Copy link
Collaborator

slorber commented Aug 17, 2020

Ahah I see it's this annoying problem again :)
Let's merge this and move on

(see facebook/react#16956)

1 similar comment
@slorber
Copy link
Collaborator

slorber commented Aug 17, 2020

Ahah I see it's this annoying problem again :)
Let's merge this and move on

(see facebook/react#16956)

@slorber slorber merged commit 51f0760 into facebook:master Aug 17, 2020
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Aug 17, 2020
@slorber
Copy link
Collaborator

slorber commented Aug 17, 2020

one suggestion culd be to handle the memoization directly in the lib, and wrap your component in a parent component whose role is to make the props "stable"

That's a bit annoying but at least it could prevent end users to have to do this themselves, so the lib is more idiomatic to use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants