Skip to content

FIREFLY-1933: Add React Compiler#1908

Open
kpuriIpac wants to merge 2 commits intodevfrom
FIREFLY-1933-react-compiler
Open

FIREFLY-1933: Add React Compiler#1908
kpuriIpac wants to merge 2 commits intodevfrom
FIREFLY-1933-react-compiler

Conversation

@kpuriIpac
Copy link
Contributor

@kpuriIpac kpuriIpac commented Feb 4, 2026

Ticket: FIREFLY-1933

  • This PR adds React Compiler to firefly:
    • I suggest devs take a look at docs: https://react.dev/learn/react-compiler/introduction
      • whenever you get a chance, not necessarily to review this PR. I think it'll be helpful to have basic understanding of React Compiler moving forward.
    • React Compiler does not automatically generate a list of components it memoizes, but you can inspect components in React DevTools -> Components and any components that have Memo :sparkles: (sparkles is the emoji with 3 stars) next to them have been memoized by React Compiler.
      • React Compiler is already used in production by several companies, including of course Meta and I think even Github (inspect & see components in React DevTools while on github.com)
    • For our existing components, React docs suggest we leave any useMemo and useCallback as is (removing them carefully, over time, is optional).
    • For newer components, we need not memoize them (but we can, as an escape hatch to provide control over which values are memoized according to react)
    • Now, as for how React Compiler works:
      • it does attempt to, theoretically, memoize every component (trying to optimize re-rendering of components essentially, and only doing so when necessary). It also attempts to optimize JSX subtrees (by caching them).
      • It will skip memoizing any components that break Rules of React. But there's no way to know which ones it skips unless we look at dev tools or get specific warnings/errors from eslint.

The bug I encountered in TAP and why React Compiler still chose to memoize this component:

  • BasicUI component (in TapViewType) was giving me issues after setting up React Compiler. After it applied memoization, a part of the component was only rendering Skeleton.
    • The issue: Earlier, we had this line in our code:
      • const capabilities = getLoadedCapability(serviceUrl);
      • capabilities is not a react state here. Pre-memoization of this component, we had a number of incidental re-renders that eventually made capabilities have a truthy value.
        • But after memoization, even after capabilities was eventually populated with the call to getLoadedCapability, this would not cause BasicUI component to re-render!
    • so turning capabilities into a react state (my code change) fixes this bug.
  • The reason why React Compiler still memoizes this component is because capabilities being falsy is not technically breaking any Rules of React (react compiler is only concerned with React props/state/context, and not global values, etc.)
    • but capabilities being falsy did lead to this being false:
      • {(capabilities && columnsModel) ?
      • and rendering <Skeleton/> instead, which is a UI bug, yes but not technically a bug React compiler would catch).

If we find other components with similar issues, they will need to be handled manually. But I am happy to report I didn't come across any more in my testing. If we do happen to find several issues in testing, we could still keep React Compiler, but adopt incremental adoption instead (https://react.dev/learn/react-compiler/incremental-adoption)

  • Basically, there's a way to apply React compiler directory by directory instead of to all of firefly at once. So we can fine tune it, keep adding directories one by one, until we've added all and find no issues with React Compiler being in firefly.

Note: I may change/remove/add some of the eslint rules.

Testing:

  • Firefly, Irsaviewer, Finderchart, Euclid, Spherex, Wise, Sofia, SHA and ZTF
  • Test the apps as extensively as possible.
    • I did a few basic searches on all apps, those worked fine. I even tested the download packager on some apps.
  • Look out for any anomalies, any components not working as expected, and please report them back here.

@kpuriIpac kpuriIpac added this to the 2026.1 milestone Feb 4, 2026
@kpuriIpac kpuriIpac self-assigned this Feb 4, 2026
Copy link
Contributor

@loitly loitly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test all of the applications, but the ones I tested are working as expected. I am glad you found and fixed the issue in TAP panel. It’s reassuring to know this was a real bug rather than a false positive from the React compiler. I am fine with merging and monitoring it closely.

Comment on lines +98 to +102
setCapabilities(getLoadedCapability(serviceUrl));
if (!isCapabilityLoaded(serviceUrl)) {
loadTapCapabilities(serviceUrl)
.then((c) => setCapabilitiesChange(c??{}))
.catch( (error) => {
setError(`Fail to retrieve capability for: ${serviceUrl}`);
});
.then((c) => setCapabilities(c ?? getLoadedCapability(serviceUrl)))
.catch(() => setError(`Fail to retrieve capability for: ${serviceUrl}`));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be like this so that it doesn't setState twice when not loaded?

if (isCapabilityLoaded(serviceUrl)) {
    setCapabilities(getLoadedCapability(serviceUrl));
} else {
    loadTapCapabilities(serviceUrl)
         .then((c) => setCapabilities(c ?? getLoadedCapability(serviceUrl)))
         .catch(() => setError(`Fail to retrieve capability for: ${serviceUrl}`));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants