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(toast-notification): clear autoclose timeout correctly #1915

Merged
merged 2 commits into from
Feb 25, 2024

Conversation

metonym
Copy link
Collaborator

@metonym metonym commented Feb 19, 2024

Does several things:

Docs

  • Adds an example using the autoclose timeout prop.

Fixes #1914

  • Fixes ToastNotification to start the timer in a reactive statement instead of onMount. This means if the timeout prop is set or changed after component initialization, the timeout should work as expected.
  • Fixes ToastNotifcation to reset the timer if the close button is clicked.

notification-autoclose.mov

src/Notification/ToastNotification.svelte Outdated Show resolved Hide resolved
Comment on lines 79 to 89
$: if (timeout) {
clearTimeout(timeoutId);
timeoutId = setTimeout(() => close(true), timeout);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • This would also restart the timeout while the toast is closed and then potentially re-fire the event.
  • Does not clear an existing timeout when set to 0 because the if would be false. Maybe just remove the condition.
  • Probably requires something like a mounted flag guard (set in onMount) to prevent running in SSR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • Could you give an example?
  • The default timeout prop value is 0, so the if statement here is intentional (0 being falsy)
  • Good flag on SSR, however, an SSR guard isn't required here as setTimeout/clearTimeout are supported in Node.js

Copy link
Contributor

@brunnerh brunnerh Feb 19, 2024

Choose a reason for hiding this comment

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

  • Just any regular set on the timeout after the toast has been closed, would trigger the reactive statement which schedules a timeout which in turn causes the event to fire. This is currently unlikely to happen intentionally because there is no way to reopen the component, as open is not accessible.
    In general the open state should probably be checked before scheduling the timeout.
  • I know why the if is there, but it should only guard the setTimeout, not the clearTimeout.
    Right now, once the timeout has started, setting it to 0 will not cause clearTimeout to run, so it's impossible to properly stop the automatic close logic once it has started.
  • Regarding SSR, one should not let timeouts like this start on the server, they serve no purpose and can only cause problems (e.g. wasting resources, causing errors and taking the server with them, keeping things alive that should not be), right now the timeout will not be cleared properly either.

By the way, I would not be opposed to just dropping this automatic close logic from the component entirely.

If open were to be exposed, it would be possible to do this in user code and having to deal with this reactively here (because properties might change) just complicates things. It is already possible to do this user code to some degree by just destroying the component after a timeout (when using the client-side component API).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've amended the code to address the following:

  • Only start the timer if open is not already false
  • Omit the clearTimeout
  • Add in an SSR guard as we don't need this to run server-side

@metonym metonym force-pushed the fix-toast-notifcation-autoclose branch from 3445d13 to 5e22a38 Compare February 24, 2024 22:36
@metonym metonym requested a review from brunnerh February 24, 2024 22:50
Copy link
Contributor

@brunnerh brunnerh left a comment

Choose a reason for hiding this comment

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

Looks better, but as it stands, the timeout has to be considered unchangeable after setting it once.

While the toast is open, the timeout cannot be disabled by setting the property to 0 and it cannot be changed to something else without starting a parallel timeout (with the old one not being cleaned up).

Changing the timeout later probably is not a common use case but this could be fixed or documented as a limitation. Could also look into a fix if you want. But I would have to look at this in more detail and do some testing. Reactive statements with multiple dependencies can be a bit tricky...


$: if (typeof window !== "undefined") {
/** Only start the timer of {@link open} has not been set to `false`. */
if (open && timeout) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
if (open && timeout) {
if (open && timeout && timeoutId === undefined) {

While the toast is open, the timeout cannot be disabled by setting the property to 0 and it cannot be changed to something else without starting a parallel timeout (with the old one not being cleaned up).

@brunnerh Thanks for the review – regarding your latest feedback, WDYT about adding an additional check that a timer has not already been set?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add this:

  $: if (typeof window !== "undefined") {
+   clearTimeout(timeoutId);

    /** Only start the timer of {@link open} has not been set to `false`. */
    if (open && timeout) {

If the timeout is changed to 0, it cancels but no new timeout is started.
If the timeout is changed to something else, it also cancels and starts a new one (unless the toast is already closed).

Test code I used for switching timeouts
<script>
    import { ToastNotification } from 'carbon-components-svelte';

	let timeout = 3000;
	let events = [];
</script>

<button on:click={() => timeout = 1000}>1s</button>
<button on:click={() => timeout = 3000}>3s</button>
<button on:click={() => timeout = 0}>disable</button>

<ToastNotification
	title="Auto-Close"
	{timeout}
	on:close={e => (events = [...events, e.detail])}>
	<svelte:fragment slot="subtitle">
		Timeout is {timeout}ms
	</svelte:fragment>
</ToastNotification>

<div>
	{#each events as event, i}
		<pre>{JSON.stringify(event, null, 2)}</pre>
	{/each}
</div>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense – thanks. Amended the commit with you as a co-author.

@metonym metonym force-pushed the fix-toast-notifcation-autoclose branch from 5e22a38 to 93fb35a Compare February 25, 2024 21:35
@metonym metonym merged commit 9aabe3c into master Feb 25, 2024
3 checks passed
@metonym metonym deleted the fix-toast-notifcation-autoclose branch February 25, 2024 21:38
@metonym
Copy link
Collaborator Author

metonym commented Feb 26, 2024

Released in v0.82.11.

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.

ToastNotification close event can fire after close due to timeout
2 participants