fix: export ToastPresentation

[kckst8]

This component is used in iTwin.js multiple times. They are referencing it from its path relative to the cjs build, which precludes anybody from using the iTwin Viewer with esm. Exporting from the barrel should allow them to import properly until a more suitable solution is added.

Checklist

• Add meaningful unit tests for your component (verify that all lines are covered)
• Verify that all existing tests pass
• Add component features demo in Storybook (different stories)
• Approve test images for new stories
• Add screenshots of the key elements of the component

[veekeys]

Hey @kckst8

Can you expand more how this "precludes anybody from using the iTwin Viewer with esm" ?
iTwinUI-react has cjs imports too (for iTwin.js bundle issues), but I do not think it blocks using iTwinUI-react with esm in anyway.

Off topic, but I also think after adding anchor points in our toasts, this presentation component might not be even needed anymore @bsteinbk, do you remember if there are any more features in toasts, which iTwinUI might not have?

[kckst8]

• @NancyMcCallB

It's not iTwinUI that precludes esm usage, but iTwin.js is importing the internal file from the cjs build of iTwinUI for a few components that are used in AppUI, and thus the iTwin Viewer (https://github.com/iTwin/itwinjs-core/blob/master/ui/appui-react/src/appui-react/messages/ToastMessage.tsx#L12). This results in runtime failures when using strictly esm tooling/bundlers (esbuild/vite).

I was hoping that the answer would be to "use this component instead". Can you give more details on how we can use an existing public component?

[aruniverse]

iTwinUI-react has cjs imports too (for iTwin.js bundle issues), but I do not think it blocks using iTwinUI-react with esm in anyway.

I feel like this has always been wrong, and something that shouldve been fixed in iTwinUi-react once iTwin.js@3.0 was released. I really dont think you guys should be importing icons from cjs, and I'm worried we're going to hit the exact same error as above. Before iTwin.js@3.0, core only shipped cjs so ofcourse your bundle size was goign tob e unreasonably large, but now that it supports esm, we shouldnt need to import icons from cjs directly.

If you want to use cjs then you're SOL and that your fault. The current approach isn't truly safe for full on esm tooling.

[veekeys]

• @NancyMcCallB

It's not iTwinUI that precludes esm usage, but iTwin.js is importing the internal file from the cjs build of iTwinUI for a few components that are used in AppUI, and thus the iTwin Viewer (https://github.com/iTwin/itwinjs-core/blob/master/ui/appui-react/src/appui-react/messages/ToastMessage.tsx#L12). This results in runtime failures when using strictly esm tooling/bundlers (esbuild/vite).

I was hoping that the answer would be to "use this component instead". Can you give more details on how we can use an existing public component?

That is interesting.. I thought cjs module is supported in esm and there should be no issue. Or is it something with paths and resolving those??? So might be just config question...
@mayank99 had vite app using iTwinUI which imports icons from cjs.

From the features of toast, what I remember the only thing we did not support back then is animating toasts to notifications center (it is in itwin.js bottom location, where notifications are collected after being shown). That is way for a temporary workaround we exported plain toast style to be used in iTwin.js. And for a reason we did not include it in barrel, cause by design you should not use toast just like that.
We have this story to show animating toasts to what ever place you want:
https://itwin.github.io/iTwinUI-react/?path=/story/core-toasts--anchor-to-button

So this would require few changes in iTwin.js and probably all the custom logic for animation and location might be removed.

iTwinUI-react has cjs imports too (for iTwin.js bundle issues), but I do not think it blocks using iTwinUI-react with esm in anyway.

I feel like this has always been wrong, and something that shouldve been fixed in iTwinUi-react once iTwin.js@3.0 was released. I really dont think you guys should be importing icons from cjs, and I'm worried we're going to hit the exact same error as above. Before iTwin.js@3.0, core only shipped cjs so ofcourse your bundle size was goign tob e unreasonably large, but now that it supports esm, we shouldnt need to import icons from cjs directly.

If you want to use cjs then you're SOL and that your fault. The current approach isn't truly safe for full on esm tooling.

@aruniverse I know this and I agree. But after facing that issue where cjs app is using iTwinUI-react and collecting all the icons, for me it seems much easier to help them by importing directly the icon versus saying back "it is your issue". I mean, we save few hunders of kbs with this one and never faced any issue (until now).

[aruniverse]

@aruniverse I know this and I agree. But after facing that issue where cjs app is using iTwinUI-react and collecting all the icons, for me it seems much easier to help them by importing directly the icon versus saying back "it is your issue". I mean, we save few hunders of kbs with this one and never faced any issue (until now).

All modern-day browsers support ESM, IE is going to die soon. I don't think we should be trying to cater to those using cjs

[mayank99]

@mayank99 had vite app using iTwinUI which imports icons from cjs.

Yes, we have multiple vite apps using iTwinUI-react without any issues. Even if we were to change those icon imports from cjs to esm, I think we'd keep the default imports (vs barrel) so that it works without tree-shaking.

---

I agree that we don't want to expose ToastPresentation in the barrel, and iTwin.js should instead try to use the Toast component with the placement and animations that we spent the time to support.

If there are any features missing that is preventing the adoption of Toast, we can try to work on those too.

[bentleyvk]

All modern-day browsers support ESM, IE is going to die soon. I don't think we should be trying to cater to those using cjs

But the problem that not all tooling fully support ESM yet like test runners.

[aruniverse]

All modern-day browsers support ESM, IE is going to die soon. I don't think we should be trying to cater to those using cjs

But the problem that not all tooling fully support ESM yet like test runners.

True, I'm not saying we don't build & deliver CJS; I just dont think we should be explicitly importing from it.

[aruniverse]

Yes, we have multiple vite apps using iTwinUI-react without any issues

But are those apps importing anything directly from cjs like in the link Kevin first shared?

[veekeys]

Yes, we have multiple vite apps using iTwinUI-react without any issues

But are those apps importing anything directly from cjs like in the link Kevin first shared?

But it is not about the app importing it, it is about the app using the package with direct cjs import, from what I understood?
In Kevins example, itwin.js build or test app does not fail. Viewer, which is using itwin.js package with that import is failing.
So kinda same connection, no?

[aruniverse]

But it is not about the app importing it, it is about the app using the package with direct cjs import, from what I understood? In Kevins example, itwin.js build or test app does not fail. Viewer, which is using itwin.js package with that import is failing. So kinda same connection, no?

iTwin.js builds and those test apps arent built with esm tooling like Vite.

@kckst8 could you share the eror you're getting?

[kckst8]

But it is not about the app importing it, it is about the app using the package with direct cjs import, from what I understood? In Kevins example, itwin.js build or test app does not fail. Viewer, which is using itwin.js package with that import is failing. So kinda same connection, no?

iTwin.js builds and those test apps arent built with esm tooling like Vite.

@kckst8 could you share the eror you're getting?

certainly there are config/plugins that could help with this, but our goal is to support esm out of the box with iTwin.js. It seems like the answer is for iTwin.js to use the toast component directly and to report back any issues

[veekeys]

But it is not about the app importing it, it is about the app using the package with direct cjs import, from what I understood? In Kevins example, itwin.js build or test app does not fail. Viewer, which is using itwin.js package with that import is failing. So kinda same connection, no?

iTwin.js builds and those test apps arent built with esm tooling like Vite.
@kckst8 could you share the eror you're getting?

certainly there are config/plugins that could help with this, but our goal is to support esm out of the box with iTwin.js. It seems like the answer is for iTwin.js to use the toast component directly and to report back any issues

Ok, I checked a little deeper.
Seems just by default, if you gonna have cjs module in some inner dep, which is not prebundled, it will not work in Vite, as it stops in barrel file and does not go to inner files to prepare the deps.
There is plugin for cjs, but I agree with Kevin you should not use that (at least for our own packages)
vitejs/vite#5308 (comment)

But this thread was more interesting, where people tried to dig deeper into it.
Just from the tech interest, could you try define that part to be pre bundled using optimizeDeps property?
vitejs/vite#3024 (comment)
Still not the expected fix, of course.

I now kinda get why cjs icons do not fail, cause there is no specific cjs import so nothing to pre bundle for vite. As a result, no failure in our use cases.

So ideally, would be good to replace toasts in itwin.js fully with toaster from iTwinUI. Just no idea, how much work there is...
Not sure we can move ToastPresentation to barrel right now as then we would have a breaking change when we want to get rid of it (for the reasons mentioned above).

@kckst8 how blocking this thing is? If the config option I suggested above helps, do you think you could use that until we get rid of ToastPresentation in iTwin.js ?

[kckst8]

But it is not about the app importing it, it is about the app using the package with direct cjs import, from what I understood? In Kevins example, itwin.js build or test app does not fail. Viewer, which is using itwin.js package with that import is failing. So kinda same connection, no?

iTwin.js builds and those test apps arent built with esm tooling like Vite.
@kckst8 could you share the eror you're getting?

certainly there are config/plugins that could help with this, but our goal is to support esm out of the box with iTwin.js. It seems like the answer is for iTwin.js to use the toast component directly and to report back any issues

Ok, I checked a little deeper. Seems just by default, if you gonna have cjs module in some inner dep, which is not prebundled, it will not work in Vite, as it stops in barrel file and does not go to inner files to prepare the deps. There is plugin for cjs, but I agree with Kevin you should not use that (at least for our own packages) vitejs/vite#5308 (comment)

But this thread was more interesting, where people tried to dig deeper into it. Just from the tech interest, could you try define that part to be pre bundled using optimizeDeps property? vitejs/vite#3024 (comment) Still not the expected fix, of course.

I now kinda get why cjs icons do not fail, cause there is no specific cjs import so nothing to pre bundle for vite. As a result, no failure in our use cases.

So ideally, would be good to replace toasts in itwin.js fully with toaster from iTwinUI. Just no idea, how much work there is... Not sure we can move ToastPresentation to barrel right now as then we would have a breaking change when we want to get rid of it (for the reasons mentioned above).

@kckst8 how blocking this thing is? If the config option I suggested above helps, do you think you could use that until we get rid of ToastPresentation in iTwin.js ?

I'm working around it by replacing the cjs import with esm for now, but the sooner we can fix this in iTwin.js the better. I'm happy to help there if needed

[NancyMcCallB]

We have 3 components in appui-react that are built in ToastPresentation. I can definitely schedule a refactor of them for next sprint.

[veekeys]

I am going to close this PR then and lets try to refactor and remove ToastPresentation from iTwin.js which should fix the issue.

[aruniverse]

What about the cjs icons?

[veekeys]

They will not break the build as I figured.
You want us to change them all too?

[aruniverse]

They're not going to break the build, but just on principle I think it's a very bad idea to be directly importing cjs here.

[veekeys]

But what is the practical con this is giving? Cause that change would be all over the places and if there is no real reason to do that, not sure we gonna do it.
Officially, cjs modules are allowed to be used in esm, so.. I do not see anything bad with it. We just do the tree shaking for everyone, so it reduces even build time, maybe?