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

Splitter state is not saved in StrictMode #3413

Closed
cfblaeb opened this issue Oct 3, 2022 · 5 comments · Fixed by #3416 or #3482
Closed

Splitter state is not saved in StrictMode #3413

cfblaeb opened this issue Oct 3, 2022 · 5 comments · Fixed by #3416 or #3482
Assignees
Labels
Type: Bug Issue contains a defect related to a specific component.
Milestone

Comments

@cfblaeb
Copy link

cfblaeb commented Oct 3, 2022

Describe the bug

Splitter sometimes overwrites local storage data.
If using <React.StrictMode> the data gets reset on refresh.
If removing StrictMode, then it works as intended.

Reproducer

https://codesandbox.io/s/primereact-test-forked-p647y5?file=/src/index.js

PrimeReact version

8.6.1

React version

18.x

Language

TypeScript

Build / Runtime

Create React App (CRA)

Browser(s)

No response

Steps to reproduce the behavior

As shown in the codesandbox, if you use StrictMode then stored data gets overwritten on refresh.
If you remove the StrictMode tags, then you can refresh and it correctly remembers you data.

Expected behavior

Don't overwrite data regardless of whether or not StrictMode is used.

@cfblaeb cfblaeb added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Oct 3, 2022
@cfblaeb cfblaeb changed the title Component: Title Splitter state is not saved in StrictMode Oct 3, 2022
@melloware
Copy link
Member

I can take a look at this. Thanks for the detailed report!

@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Oct 4, 2022
@melloware melloware self-assigned this Oct 4, 2022
@melloware melloware added this to the 8.6.2 milestone Oct 4, 2022
melloware added a commit to melloware/primereact that referenced this issue Oct 4, 2022
@kalinkrustev
Copy link
Contributor

I do not think it is related only to StrictMode. It is always overwritten on rerender.

Open the reproducer URL and then the Developer Tools.
Resize the splitter and observe the local storage is updated with the new size of the panels.
Click the button and the local storage will be overwritten with [50, 50].

I was going to propose an alternative fix, which I think avoids calling restoreState on each render.

            if (props.children && props.children.length) {
                let initialized = isStateful && panelSizes.current; // this line is changed

                if (isStateful && !mounted.current) {
                    initialized = restoreState();
                }

@melloware
Copy link
Member

@kalinkrustev sure thing please submit the PR.

@kalinkrustev
Copy link
Contributor

Sorry, I did not notice the switch from React.useEffect to useMountEffect, so probably there is not restoreState on each render.

Still I think a better fix is to restoreState() when one of the properties stateKey or stateStorage has changed, instead of doing this only once after mount.

See #3482. I have not tested it yet, but it should work better than the original fix and I think it is a simpler approach.

@melloware
Copy link
Member

Looks good there is one build error but let me know how your testing goes.

@melloware melloware reopened this Oct 17, 2022
@github-actions github-actions bot added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Oct 17, 2022
@melloware melloware removed the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Oct 17, 2022
melloware pushed a commit that referenced this issue Oct 18, 2022
* Fix #3413: Splitter restoreState dependencies

* fix: move Splitter panel sizes in state

* chore: prettier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issue contains a defect related to a specific component.
Projects
None yet
3 participants