rusackas commented on PR #35777: URL: https://github.com/apache/superset/pull/35777#issuecomment-3543365518
Hey again, I was looking at this a bit over the weekend, and wondering if this is the right approach for this, really. It seems that there are a few things going on which might be problematic: 1) The polling, and using the window object, rather than using react router for these state changes 2) Using localstorage rather than sessionstorage... it seems the latter might actually be more applicable here and provide less bloat. 3) Just unsure of the event listeners in general and whether that's the right angle. Had a discussion with Copilot today about this (not my preferred agent, but it's here in GitHub) and it suggested to avoid per-component global listeners and polling by handling cleanup at the router level using React Router's location change detection. That keeps ChartRenderer focused on per-chart state (save/load to localStorage or sessionStorage) and puts any cross-chart cleanup in one tiny place that only runs when the app actually navigates. This would: • Keep ChartRenderer's per-chart save/load behavior (setItem/getItem) as-is. Remove the global counters, interval, and add/removeEventListener logic from ChartRenderer. • Add a single small component mounted once inside the Router that runs cleanup when the location pathname changes (useLocation / history.listen). No polling, no per-instance listeners, only one effect. I'll paste it's suggestions in another comment for your consideration... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
