Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
4 tasks done
csr632 opened this issue Feb 24, 2024 · 5 comments · May be fixed by #2445
Open
4 tasks done

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

csr632 opened this issue Feb 24, 2024 · 5 comments · May be fixed by #2445
Labels
🗄 area/interface This affects the public interface 🤞 phase/open Post is being triaged manually 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem

Comments

@csr632
Copy link

csr632 commented Feb 24, 2024

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
Copy link
Author

csr632 commented Feb 24, 2024

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
Copy link
Author

csr632 commented Feb 24, 2024

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.

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Feb 24, 2024

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.

@csr632
Copy link
Author

csr632 commented Feb 24, 2024

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.

@remcohaszing
Copy link
Member

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 remcohaszing added 🐛 type/bug This is a problem 🗄 area/interface This affects the public interface 👶 semver/patch This is a backwards-compatible fix 🤞 phase/open Post is being triaged manually labels Feb 25, 2024
remcohaszing added a commit that referenced this issue Feb 25, 2024
If the user provides a `wrapper` component, `_createMdxContent` was
treated as a JSX component. Otherwise it was invoked as a function.
Because `_createMdxContent` uses a hook, this hooks becomes part of the
`MDXContent` component conditionally. This breaks React’s rule of hooks.

Closes #2444
@remcohaszing remcohaszing linked a pull request Feb 25, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🤞 phase/open Post is being triaged manually 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem
Development

Successfully merging a pull request may close this issue.

3 participants