facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
223.76k stars 45.56k forks source link

[React Compiler] React Fast Refresh Compatibility Issue #29115

Open daniel-nagy opened 2 weeks ago

daniel-nagy commented 2 weeks ago

Summary

When using the React Compiler, Fast Refresh is broken when importing a non-JavaScript file, .e.g.

import content from "./content.md";

export const App = () => {
  return <div>{content}</div>;
};

In this example, if the content of the markdown file changes, then the app does not update without a page refresh. A repo for this example can be found at https://github.com/daniel-nagy/react-compiler-bug.

From a quick investigation, the issue appears to be that the compiler memo cache is global and that it does not treat imported values as mutable.

josephsavona commented 2 weeks ago

Thanks for posting! In React, globals are expected to not change, but this example shows that can break in fast refresh mode. We already have dev-mode support for fast refresh where we reset the memo cache if the source of the file changes, maybe we can also clear it on changes to referenced globals.

Out of curiosity, does the example work if you use useMemo(() => contents, []) ? That’s effectively what the compiler is doing.

daniel-nagy commented 2 weeks ago

Out of curiosity, does the example work if you use useMemo(() => contents, []) ?

Yes, without the compiler and with useMemo e.g.

import { useMemo } from "react";
import content from "./content.md";

export const App = () => {
  return <div>{useMemo(() => content, [])}</div>;
};

Fast refresh works.

From what I can tell, every time the content of an imported file changes, Vite will replace every file that imports that file with a new file. For example, this is what I see in the browser's developer tools after editing the markdown file a bunch of times.

image

The problem is that the compiler memo cache is external to the file. So even though the file is replaced, the cache still has the old value and becomes stale. If the cache were local to the file, I believe the problem would go away.

On a somewhat related but also unrelated note, I'm a little surprised the compiler uses a memo cache instead of just hoisting the static bits. In this case, no optimization is actually required by the compiler because the import is outside the component.

josephsavona commented 2 weeks ago

Thanks for confirming. What’s happening here is that the compiler is memoizing the <div> wrapping the contents. I have an idea for what we can do here to make this work - basically consider global as reactive in dev mode. This would incur some extra if/else checks for dependencies in dev mode but shouldn’t otherwise change behavior.

I’ll experiment w this.

josephsavona commented 1 week ago

I added some test fixtures for this in https://github.com/facebook/react/pull/29175 — not a fix yet, but just reproducing the problem in tests.

josephsavona commented 1 week ago

As I thought about how we might make this case work, it's pretty tricky. The challenge is that in production we don't want to incur the overhead of comparing whether known-constants have changed (they're constants!). But in development, you might change the value of a constant and it's reasonable to expect that to show up with fast refresh. We already reset the cache if the source file itself changes, so we're really just talking about contents from other files.

A naive approach would be to reset the cache if any constants have changed — but we have to think through the implications. It's important that behavior is consistent between development and production, and we wouldn't want fast refresh support to mask bugs in your application (by resetting the cache more often) that then show up in production (as values not appearing to update).

We'll continue to explore this and also debug the repro you sent. We actually wouldn't expect the useMemo version to reset, so this may be a plugin ordering issue.