microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.53k stars 2.74k forks source link

`registerIcons` storing its icons in the `window` object throws errors if the icon being registered calls a hook, and there are multiple React apps on the page trying to render the icon #21615

Closed mbest closed 1 year ago

mbest commented 2 years ago

Copied from #17326:

Environment Information

(I'm going to create my own structure for this bug, as it isn't reproducible in Codepen and is a bit of a niche bug).

Issue

The issue is prefaced with a question - does Fluent support multiple React apps on the page at the same time? I believe the answer is yes, I'll assume as much for this bug at least, but if it expressly doesn't support this, then this isn't an issue.

Due to registerIcons storing its icons in the window object, now that fluentui/react-icons-mdl2 has a call to a hook in its icons, if I register an icon from that package in two different apps and try to render it, an error will be thrown.

Let's say AppX and AppY both register "screentime": <ScreenTimeIcon /> (an icon that's in react-icons-mdl2 but not in the default icon set). AppX happens to register its icons first, so the actual component stored in the window object comes from this app. When AppY tries to register the icon, it just gets a warning - the icon in the window object was still registered by AppX.

If I were to then try and actually render an <Icon /> with the iconName of "screentime" in AppY, React will throw a "Minified error 321 - No hooks calls outside of a component". This is because even though the useIconSubset hook call is safely in a component, because the component is coming from an entirely different app, React thinks that the hook is being called unsafely.

I have a whole heck of a lot more info about this, feel free to reach out to me (alias = @ibrosen ) and I can provide screenshots / stack traces etc.

Possible solutions

The best solution would be to not store icons in the window object

Priorities and help requested:

Are you willing to submit a PR to fix? (Yes, No) Maybe, it will be a lot of work to re-imagine icon storage though

Requested priority: (Blocking, High, Normal, Low) Normal. This did cause a live-site incident for us, but we've mitigated it.

Products/sites affected: (if applicable) account.microsoft.com

gouttierre commented 2 years ago

Thanks for filing this issue with us and sorry about the frustration caused. The team will investigate this issue but to set expectations the developers will only review this issue once personal capacity allows. I do see you maybe/ or might be willing to contribute to this issue? If you have a current solution and would like to share your PR with us, our team will gladly review it.

@GeoffCoxMSFT - Would you be able to confirm if this is a regression or behavior issue? cc @tomi-msft

tudorpopams commented 2 years ago

Let's make sure this doesn't affect future versions of the icon library and see if there's a straight forward fix for react-icons-mdl2.

msft-fluent-ui-bot commented 1 year ago

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.