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

Editing notifications (or recreating them) #1667

Closed
max06 opened this issue Apr 14, 2024 · 13 comments · Fixed by #1668 or #1673
Closed

Editing notifications (or recreating them) #1667

max06 opened this issue Apr 14, 2024 · 13 comments · Fixed by #1668 or #1673
Labels
bug Something isn't working

Comments

@max06
Copy link

max06 commented Apr 14, 2024

Environment

  • Operating System: Linux
  • Node Version: v20.11.0
  • Nuxt Version: 3.11.2
  • CLI Version: 3.11.1
  • Nitro Version: 2.9.6
  • Package Manager: bun@1.1.3
  • Builder: -
  • User Config: devtools, extends, modules, nitro, tailwindcss, ui
  • Runtime Modules: @nuxt/ui, @nuxtjs/color-mode, @nuxtjs/tailwindcss, @pinia/nuxt@^0.5.1, nuxt-monaco-editor@^1.2.7
  • Build Modules: -

Version

v2.15.0

Reproduction

https://stackblitz.com/edit/nuxt-ui-vk1zzz?file=components%2Fdemo.vue

Description

I want to update notifications based on the current state of a routine.

Since there's no function for that, I went with reusing the notification id (after deleting the current one).

The reproduction shows a notification with 10 seconds timeout, sleeps 5 seconds, removes the notification and creates a new one with the same id, but different options (like a 60 seconds timeout).

-> New notification shows up, but only has 5 seconds left (also indicated by the timeout bar).

Additional context

No response

Logs

No response

@max06 max06 added the bug Something isn't working label Apr 14, 2024
@max06
Copy link
Author

max06 commented Apr 15, 2024

Sorry for bringing this up again - thanks for the new function, but the bug described in here is not fixed by that :(

@noook
Copy link
Collaborator

noook commented Apr 15, 2024

@max06 Are you on the edge channel ? If not, it has just not been released yet.

Note that if you change the id, it will recreate the notification instead of editing it

@max06
Copy link
Author

max06 commented Apr 15, 2024

@noook I'm testing with

"@nuxt/ui-edge@npm:@nuxt/ui-edge@latest":
  resolved "https://registry.npmjs.org/@nuxt/ui-edge/-/ui-edge-2.15.2-28552830.ed3a3ba.tgz"

It has the update-function, so I'd think it's fine.

I did not change the id. Defined as const at the beginning of my routine, and reused till the end.

Edit: Just in case it's just me...

Recording.2024-04-15.130537-1.mp4

Copy link
Collaborator

noook commented Apr 15, 2024

@noook
Copy link
Collaborator

noook commented Apr 15, 2024

Oh I see what happened. In your reproduction, you remove the notification, and recreate it. You recreate it with a lower timeout to simulate the remaining time of your notification.

What happens if you update the timeout (in this situation, reducing it) it will just lower the timeout of the notification from 10 seconds to 6, so you kind of remove 4 seconds off the notification's timeout when updating. Just don't update it, your sleep does the job

@max06
Copy link
Author

max06 commented Apr 15, 2024

I actually increased the timeout to 60 seconds, expecting either 55 or 60 seconds - not 5 :)

@max06
Copy link
Author

max06 commented Apr 15, 2024

To give you a rough idea: Stage 1 creates some resource definitions. This takes just some seconds and it either works or it doesn't. The second stage then waits for a specific event to happen, which can take up to a minute.

Copy link
Collaborator

noook commented Apr 15, 2024

I see. I don't think the update method suits your needs then. In this case, maybe recreating the notification is indeed the best solution for the moment.

However, I understand there might be unwanted behavior when updating a notification's timeout.

@benjamincanac should I reset the notification's timer with a watcher on this specific's property update ?

@max06
Copy link
Author

max06 commented Apr 15, 2024

The weird thing is: I thought removing and recreating the notification would cause the timer to start from the beginning, since it should be (technically) a new element. At the moment I'm not so sure if the element gets removed at all on calling remove, or if it's just hidden.

Copy link
Collaborator

noook commented Apr 15, 2024

My guess is, there is no tick between the moment you remove and add the notification again, so the component is still mounted and the timeout not reset.

As you can see, the timer starts on the UNotification mount

onMounted(() => {
if (!props.timeout) {
return
}
timer = useTimer(() => {
onClose()
}, props.timeout)
watchEffect(() => {
remaining.value = timer.remaining.value
})
})
onUnmounted(() => {
if (timer) {
timer.stop()
}
})

Try await nextTick() between toast.remove() and toast.add()

@max06
Copy link
Author

max06 commented Apr 15, 2024

So, a race condition! That nextTick() helped and allows me to move on, great!

I'd probably suggest something like this then:

toast.update(id: string, notification: Partial<Notification>, resetTimeout: boolean)

@noook
Copy link
Collaborator

noook commented Apr 15, 2024

Not exactly a race condition. Vue will not re-render the element until all the values have been re-evaluated and not in a dirty state (one change can trigger the change of another variable multiple times, but we only want to re-render once). Once the variables's state is "clean", Vue knows it can apply the changes.

In your situation, this is what happened. You removed the element, but you also added it back at the same time. The variable state was evaluated to "dirty" when you called remove (= no re-render), then added back (still "dirty"). All variables have been changed, so it is "clean" again. And the UNotification didn't have the need to re-render.

Calling nextTick means "Susped this function execution until the next render has completed", so you waited for the notification to be removed, and finally re-added it which triggered the mount lifecycle event, and correctly applied the new timeout.

I don't think setting a resetTimeout parameter is the solution. As I said, updating the timeout without being aware of this caveat is an unwanted behavior. In general, when a user updates a timeout, it should behave as you would expect it to behave, just like in your situation. Resetting the timeout should be the default behavior.

@max06
Copy link
Author

max06 commented Apr 15, 2024

@noook Updated! Looks great, much better! 🙇🏼

Thanks a lot! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants