mdx icon indicating copy to clipboard operation
mdx copied to clipboard

react hmr (fast refresh) breaks when deleting all content of a mdx file

Open csr632 opened this issue 1 year ago • 5 comments

Initial checklist

Affected packages and versions

3.0.1

Link to runnable example

https://stackblitz.com/edit/vitejs-vite-sorj9w?file=src%2Ftest.mdx,vite.config.ts&terminal=dev

Steps to reproduce

  1. Go to https://stackblitz.com/edit/vitejs-vite-sorj9w?file=src%2Ftest.mdx,vite.config.ts&terminal=dev (run npm run dev if it is not auto run)
  2. Delete all content of test.mdx
  3. The app is broken after hmr: image

Expected behavior

The app should not be broken.

Actual behavior

The app is broken.

Runtime

Node v20

Package manager

npm v10

OS

macOS

Build and bundle tools

Vite

csr632 avatar Feb 24 '24 16:02 csr632

Notice the key point to reproduce this bug: in vite.config.ts, I configure the @vitejs/plugin-react plugin to handle .mdx file, so it will do react-refresh transform to the .mdx file.

Then checkout the test.mdx loaded in the browser: notice that at first there are _provideComponents() calls. But when we delete all content of the test.mdx source file, the _provideComponents() calls are gone (in the hmr update file): 未命名

Because _provideComponents() contains some calls to react hooks, it violates the react hook rules during hmr.

csr632 avatar Feb 24 '24 16:02 csr632

I think we can fix it by turning this _createMdxContent() call into a component render (<_createMdxContent />):

So that the react hooks belongs to a nested component instead of the exported (fast-refreshed) component.

csr632 avatar Feb 24 '24 16:02 csr632

Welcome @csr632!

Could you explain how you see this as as breaking the rule of hooks? Those rules are (https://legacy.reactjs.org/docs/hooks-rules.html)

  1. Only Call Hooks at the Top Level. Don’t call Hooks inside loops, conditions, or nested functions
  2. Only Call Hooks from React Functions. Don’t call Hooks from regular JavaScript functions.

This call you reference is:

  1. Made at the top level
  2. Made from inside a react component

It doesn't feel like a violation of the rule of hooks. It feels more like a bug in vite HMR.

ChristianMurphy avatar Feb 24 '24 16:02 ChristianMurphy

Hi @ChristianMurphy ! I think it breaks the rule of hooks because it calls the hooks conditionally (in a very tricky way). When the hmr happened, the bundler will re-fetch the module (test.mdx?t=xxx) and run it. Then the react fast-refresh mechanism comes in to work. It treats the exported compent of test.mdx?t=xxx the same component (but a newer version) as the one exported by previoustest.mdx. Then it finds out that there are different number of hook calls inside the "same" component.

The other hook rule it violates is that, _createMdxContent() is a not a valid hook name. It should start with useXXX. The React fast refresh transform plugin rely on this to detect whether the component have hook calls (and whether it should handle the hook call changes of it. That's why we can add/remove hook call during dev in normal case.) In this case, the fast refresh doesn't realize there are hook calls inside the MDXContent component. So I guess wrap the _createMdxContent() call into a funtion named useXXX can also fix the bug.

csr632 avatar Feb 24 '24 16:02 csr632

Ah yes, I see the problem. A very similar problem happens when the value of wrapper changes between undefined and a defined value in the MDX provider. https://stackblitz.com/edit/vitejs-vite-kt1c1v?file=src%2FApp.tsx

Hooks may only be called from other hooks (function starting with use) or React components. However, if wrapper is undefined, _createMdxContent is called as a function which is neither a component nor a hook. Static analysis tools (which I imagine includes hot reloading) may have a problem with that.

The solution is as @csr632 proposes, to change the _createMdxContent(props) call into a JSX expression (<_createMdxContent {...props} />)

remcohaszing avatar Feb 25 '24 12:02 remcohaszing