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.

Sign up for GitHub

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

Jump to bottom

Toast/Messages: Fix updates incorrectly updating older version of the state #4350

Closed
ecstrema opened this issue May 7, 2023 · 6 comments · Fixed by #4349 or #4364
Closed

Toast/Messages: Fix updates incorrectly updating older version of the state #4350

ecstrema opened this issue May 7, 2023 · 6 comments · Fixed by #4349 or #4364
Assignees
@ecstrema
Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Milestone
9.4.0

Comments

@ecstrema
Copy link
Contributor

ecstrema commented May 7, 2023 •
edited
Loading

Describe the bug

Without the fix, instead of being added, every update clears the state.

Before the fix:

before.mp4

After the fix:

after.mp4

Reproducer

codesandbox link

PrimeReact version

9.3.1

React version

18.x

Language

TypeScript

Build / Runtime

Next.js

Expected behavior

I don't know why it happens. I will try to investigate, but what I know for sure is that pr #4349 fixes it.
@ecstrema ecstrema added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label May 7, 2023
@ecstrema ecstrema mentioned this issue May 7, 2023
Merged
@ecstrema
Copy link
Contributor Author

ecstrema commented May 7, 2023 •
edited
Loading

Reproduction added

@ecstrema
Copy link
Contributor Author

ecstrema commented May 7, 2023 •
edited
Loading

In the repro, changing useToast to return the ref instead of the value fixes it. I guess this is one of the hooks rule I am always forgetting about... I'd be glad to have an explainer though.

@melloware
Copy link
Member

Let me investigate your reproducer

@melloware
Copy link
Member

melloware commented May 8, 2023 •
edited
Loading

OK yeah what you are doign seems way overly complex for what a Toast is needed for.

You are right.... I fixed your reproducer the .current method returns the current instance of the ref at that time so you should delay calling current until you need it.

https://codesandbox.io/s/primereact-demo-forked-0pum4q?file=/src/App.js

@melloware melloware added Resolution: By Design The behavior in the issue is by design and the component exhibits the expected behavior and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels May 8, 2023
@melloware melloware closed this as not planned Won't fix, can't repro, duplicate, stale May 8, 2023
@ecstrema
Copy link
Contributor Author

ecstrema commented May 8, 2023

You are right.... I fixed your reproducer the .current method returns the current instance of the ref at that time so you should delay calling current until you need it.

There is something inherently safer in always using setState with a function, to always use the latest version of the state.

That's true, but it still fixes the issue with no additionnal overhead. Why not accept the PR?

OK yeah what you are doign seems way overly complex for what a Toast is needed for.

Toasts are needed throughout the application. It makes sense to me to have a single ToastProvider. A lot of UI frameworks do that, as it removes a lot of the boilerplate code to show a Toast.

@melloware melloware added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add and removed Resolution: By Design The behavior in the issue is by design and the component exhibits the expected behavior labels May 8, 2023
@melloware melloware reopened this May 8, 2023
@melloware
Copy link
Member

I re-opened the issue and sent you a code review comment!

@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 May 8, 2023
@melloware melloware changed the title Toast: Fix updates incorrectly updating older version of the state Toast/Messages: Fix updates incorrectly updating older version of the state May 8, 2023
@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 May 8, 2023
@melloware melloware added this to the 9.4.0 milestone May 8, 2023
melloware added a commit that referenced this issue May 9, 2023
@ecstrema @melloware
Fixes #4350: updates incorrectly replacing old state instead of actual (
d168af8 
#4349)

* fix: updates incorrectly replacing old state instead of actual.

* fix: follow up for #4349: apply fix to messages as well

* Update Messages.js

* Update Toast.js

---------

Co-authored-by: Melloware <[email protected]>
melloware added a commit to melloware/primereact that referenced this issue May 9, 2023
@melloware
Fix primefaces#4350: Update messages examples and useMountEffect
c972f80
@melloware melloware mentioned this issue May 9, 2023
Merged
melloware added a commit that referenced this issue May 9, 2023
@melloware
Fix #4350: Update messages examples and useMountEffect (#4364)
8f75eec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ecstrema ecstrema

Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Projects
None yet
Milestone
9.4.0
2 participants
@melloware @ecstrema