search

iTwin / appui

Monorepo for iTwin.js AppUi
MIT License
8 stars 2 forks source link

Console error message shows memory leak #976

Closed arome closed 2 months ago

arome commented 2 months ago

Describe the bug

When running in development, right after launching the app and being presented with the iModel Selection screen, I see this error appearing in the console.

image

A similar error then appears again after selecting an iModel

image

To Reproduce

  1. Run the booster app from VS Code.
  2. After the app launches, open the chrome devTools (Mac: Command + Option + I, Windows/Linux: F12 or Control + Shift + I)
  3. You'll see in the console the error message appearing

Expected Behavior

I'm expecting the console to not have any errors thrown.

Screenshots

No response

Desktop (please complete the applicable information)

Additional context

No response

ignas-k commented 2 months ago

Could you provide the specific iModel which you open to reproduce the second warning? I can reproduce the first one just fine but I don't get the second one.

ignas-k commented 2 months ago

I looked into the issue. Seems like the warning is coming from studio's code and it is thrown by iTwinUI. I could not reproduce the second warning message but I got some another similar warning (component from another iTwin repository). However, the question is: is this issue urgent for you? This is not even an error but a warning and everythings seems to work just fine. Besides that this warning is removed in React 18 (and studio are already planning on upgrading) because it produces mostly false positives and the workarounds make the code worse (admitted by React developers). Link from the React documentation about this topic: https://react.dev/blog/2022/03/08/react-18-upgrade-guide#react (third entry, also has a link to the PR with examples and justification). Would you mind waiting for React 18 upgrade or do you need this fixed now?

mayank99 commented 2 months ago

I think this issue is safe to close, because it happens only in React 17 and only during development when using Strict Mode. We could add workarounds, but since this warning doesn't happen in the latest React version and it does not cause any real issues, it's not worth adding extra code. (Further, Booster and iTwin Studio should upgrade to React 18 ASAP because React 17 will likely not be supported by the next version of AppUI and the new Kiwi design system.)

In Strict Mode, React unmounts/remounts the components, which results in iTwinUI's setState call executing after the component is unmounted. As explained by React core team member, this is a false positive — setState called on an unmounted component gets simply ignored. There is no memory leak because we aren't setting up any subscriptions.

arome commented 2 months ago

thanks for looking into it. It's definitely not a blocker and not urgent for me. I was just pointing it out in case this was causing memory leak in the app and since its a false positive and the error is expected to be fixed with a React update then lets wait for that.