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

Toast is not shown during first trigger #1005

Closed
igneqas opened this issue Jan 17, 2023 · 10 comments
Closed

Toast is not shown during first trigger #1005

igneqas opened this issue Jan 17, 2023 · 10 comments
Labels
bug Something isn't working has workaround This issue has a workaround or is possible to achieve in user-land. react Needs change in react package

Comments

@igneqas
Copy link

igneqas commented Jan 17, 2023

Describe the bug (actual behavior)

After upgrading to iTwinUI v2, toasts do not show up the first time they are triggered but it works fine from the second time (only in our project). I tried recreating the problem in a new project but it works correctly there.

Expected behavior

Toast should be visible the first time it is triggered.

Reproduction

Could not be reproduced on a new project.

Additional information

iTwinUI - 2.2.0
React - 18

We did not modify our code for toasts after upgrading to v2. Also, the code is executed and toast is being created during the first trigger but it just doesn't show up. Our code is simple:

const toastSuccess = (message: string) => {
    toaster.closeAll();
    toaster.positive(message, { type: "temporary" });
  };

I guess this is some kind of timing issue, because, for example, if I add a 2 second sleep function between toaster.closeAll() and toaster.positive() it does show up during the first trigger (or calling toaster.closeAll() during the first render of the app).

@igneqas igneqas added the bug Something isn't working label Jan 17, 2023
@gretanausedaite gretanausedaite added the react Needs change in react package label Jan 17, 2023
@mayank99 mayank99 added the has workaround This issue has a workaround or is possible to achieve in user-land. label Jan 18, 2023
@mayank99
Copy link
Contributor

mayank99 commented Jan 18, 2023

We talked offline and this seems to be because of react 18 (and 17) batching updates. Wrapping toaster.closeAll in flushSync fixes it.

I thought about maybe adding flushSync in our toaster code but decided against it because of how much react discourages using it and because this issue is not common enough.

If we get more users who face this issue in the future, we can revisit this.

@mayank99 mayank99 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 18, 2023
@veekeys
Copy link
Member

veekeys commented Jan 18, 2023

I think we also had this issue in our project, but solution added was to call setSettings... I would need to double check, if there is any more info I can gather on this. (react 18 with iTwinUI v1)

@mayank99
Copy link
Contributor

solution added was to call setSettings

that's really strange. it might have something to do with the async nature of asyncInit (which is being called in setSettings and createToast). i wonder if should make those functions async so that you can await them in application code like this:

(async () => {
  await toaster.closeAll();
  await toaster.positive();
  await toaster.informational();
})();

@mayank99
Copy link
Contributor

@igneqas I've created a PR #1025 which will allow you to do this:

const toastSuccess = async (message: string) => {
  await toaster.closeAll();
  toaster.positive(message, { type: "temporary" });
};

But I have no way to test this because I can't reproduce your issue. Can you test in your app by changing the code inside your node_modules? I can also make a snapshot release if that's easier for you.

@veekeys Would be nice if you could also test, but no worries if not.

@veekeys
Copy link
Member

veekeys commented Jan 24, 2023

@mayank99 Give me a day or so as its busy week, I will try to test it in the code we have faced this issue!

@igneqas
Copy link
Author

igneqas commented Jan 24, 2023

@mayank99 Hey, it would be great if you could create a snapshot for me, thanks.

@mayank99
Copy link
Contributor

Released it in the dev channel. You can try it with npm i @itwin/itwinui-react@dev or pin the exact version:

"@itwin/itwinui-react": "2.3.1-dev.0"

@igneqas
Copy link
Author

igneqas commented Jan 25, 2023

I tested the new changes and it didn't change anything. The problem is still there.

@mayank99
Copy link
Contributor

That's unfortunate. well, i can't repro and least there's two workarounds now, so i'll close this issue.

I expect that at some point we will refactor the toaster anyway.

@mayank99 mayank99 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 25, 2023
@mayank99
Copy link
Contributor

fyi, all async behavior has been removed in #1351 which will be released in 3.0.

this snippet now works without hacks:

const toaster = useToaster();

<Button onClick={() => toaster.positive('hello')}>Show</Button>
<Button
  onClick={() => {
    toaster.closeAll();
    toaster.positive('hello');
  }}
>
  Close all
</Button>
Screen.Recording.2023-06-13.at.4.50.19.PM.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has workaround This issue has a workaround or is possible to achieve in user-land. react Needs change in react package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants