rusackas commented on PR #39458: URL: https://github.com/apache/superset/pull/39458#issuecomment-4662933237
@aminghadersohi thanks for the thorough review! - **MEDIUM-1 (propsRef mutated in render):** Fixed in b69bbcade2 — moved the assignment into a `useLayoutEffect` so it only runs for committed renders (Concurrent-Mode safe) and is in place before the passive unmount effect reads it. - **NIT-1 (test mocks not reset):** Fixed in the same commit — added a `beforeEach` that clears `renderFn`/`willUnmountCb` so the call-count assertions are order-independent. - **MEDIUM-2 (ComponentClass → ComponentType public-API widening):** I want to leave this one for a conscious call rather than silently changing it. As you verified, no in-repo caller annotates against `ComponentClass<...>`, so the monorepo is fine; the only exposure is external consumers of `@superset-ui/core` who explicitly typed a variable as `ComponentClass`. The widening is what makes the function-component conversion possible at all, and `ReactifiedComponent<Props>` is already exported as the narrowing path. My inclination is to keep the widened type and add a short UPDATING.md note rather than block on a major bump — but I would like your read on whether that warrants a semver note before I touch the comment wording. Happy to go either way. -- 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]
