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

HMR didn't work for dynamic imported modules (React) #20

Closed
Alex-Ferreli opened this issue Mar 26, 2021 · 16 comments · Fixed by #79
Closed

HMR didn't work for dynamic imported modules (React) #20

Alex-Ferreli opened this issue Mar 26, 2021 · 16 comments · Fixed by #79
Labels
feat: hmr p3-minor-bug 🔨 An edge case that only affects very specific usage (priority)

Comments

@Alex-Ferreli
Copy link

Describe the bug

Plugin-react-refresh didn't refresh dynamic imported modules.

Reproduction

Repository to reproduce:
https://github.com/Alex-Ferreli/vite-react-hmr-dynamic-import

It's the default react-ts template with a dynamic import of a React component.

System Info

Output of npx envinfo --system --npmPackages vite,@vitejs/plugin-vue --binaries --browsers:

System:
    OS: Linux 5.8 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (8) x64 Intel(R) Core(TM) i7-10510U CPU @ 1.80GHz
    Memory: 404.95 MB / 15.43 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 15.5.0 - ~/.nvm/versions/node/v15.5.0/bin/node
    Yarn: 1.22.5 - /usr/bin/yarn
    npm: 7.3.0 - ~/.nvm/versions/node/v15.5.0/bin/npm
  Browsers:
    Chrome: 89.0.4389.90
    Firefox: 86.0.1
  npmPackages:
    vite: ^2.1.3 => 2.1.3

Used package manager: yarn

Logs

The HMR in console print the parent file:
[vite] hot updated: /src/App.tsx

Before submitting the issue, please make sure you do the following

  • [ x] Read the Contributing Guidelines.
  • [ x] Read the docs.
  • [ x] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • [ x] Provide a description in this issue that describes the bug.
  • [ x] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
  • [ x] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
@patak-dev patak-dev added the bug Something isn't working label Mar 27, 2021
@davidbonnet
Copy link

You confirm that this is a Vite bug and not an upstream one?
IMHO, this issue is not a minor one: it this is a quite common issue for React apps for splitting bundles.

@Shinigami92
Copy link
Member

@davidbonnet it's quite arguable what's a minor and what's an upstream bug
This doesn't affect the whole Vite ecosystem but the react ecosystem

We currently only have these two p3-* labels, we may discuss internally if we want another label for each framework 🤔

@Alex-Ferreli
Copy link
Author

I have found the solution. I have updated the repository blocking version on package.json to reproduce the solution with the exact version of all packages of the initial issue.

This is the fix:
Alex-Ferreli/vite-react-hmr-dynamic-import@de1957a

Really strange bug, seems that the dynamically imported component must be created and default exported separately.

@davidbonnet
Copy link

davidbonnet commented Apr 5, 2021

As I suspected, this an upstream issue located in react-refresh. Might be good to upgrade it to v0.10.0 though.

For now, the issue raised by @Alex-Ferreli is intentional ("Anonymous direct exports […] are currently ignored."):
https://github.com/facebook/react/blob/a817840ea7d3818c0590cccb9159b13220f4fdb4/packages/react-refresh/src/ReactFreshBabelPlugin.js#L430-L440

      ExportDefaultDeclaration(path) {
        const node = path.node;
        const decl = node.declaration;
        const declPath = path.get('declaration');
        if (decl.type !== 'CallExpression') {
          // For now, we only support possible HOC calls here.
          // Named function declarations are handled in FunctionDeclaration.
          // Anonymous direct exports like export default function() {}
          // are currently ignored.
          return;
        }

Ironically, something like:

function identity(value) {
  return value;
}

export default identity(() => {
  return <div>TRY TO CHANGE ME</div>;
});

…would work. It is however considered as bad practice to export unnamed components.

The issue I witnessed is slightly different, and still located in react-refresh (facebook/react#21179).

IMHO, we can close this and re-open a new issue to upgrade the dependency once these are fixed.

@patak-dev patak-dev removed the bug Something isn't working label Apr 5, 2021
@patak-dev
Copy link
Member

Thanks for digging into this @davidbonnet, I think we can leave this one open with the bug: upstream label. Let's update the dependency and close it when there is a release

@davidbonnet
Copy link

Opened facebook/react#21181 to that effect.

@Alex-Ferreli
Copy link
Author

Thanks @davidbonnet for the clarification.

Maybe it's better to write it on the documentation, because this is the first time that i found and use this workaround, it's not specified anywhere.

@davidbonnet
Copy link

IMHO, it should at least refresh the page. I didn't have time yet to investigate the origin of that issue:
facebook/react#21181 (comment).

@xiaomuzizier
Copy link

I have same issue

@xiaomuzizier
Copy link

xiaomuzizier commented Apr 15, 2021

image

image

please see it. Have I same issue with you?

@hyf0
Copy link

hyf0 commented Aug 14, 2021

Not a bug. see vitejs/vite#4298.

@wzono
Copy link

wzono commented Dec 26, 2021

Same issue, any progress for it?

@penx
Copy link

penx commented Feb 6, 2022

In order to fix hot reloading, say you have a React component:

export default () => <div />

You need to assign this to a named constant first, i.e.

const MyComponent () => <div />
export default MyComponent

@sapphi-red
Copy link
Member

I confirmed this still does not work. (Vite 4.0.2 + plugin-react 3.0.0)
https://stackblitz.com/edit/github-sncxvn?file=src%2FNoHMRHere.tsx

HMR not happening is expected due to the limitation of react refresh, but the full reload should happen.

@sapphi-red sapphi-red added feat: hmr p3-minor-bug 🔨 An edge case that only affects very specific usage (priority) labels Dec 19, 2022
@Alex-Ferreli
Copy link
Author

Do not export the component as anonymous function, assign it to a variable first, like written in this comment:
#20 (comment)

@ArnaudBarre
Copy link
Member

This issue will be fixed by #79. Waiting for Vite 4.1 to enable this change

Reminder: This is not a reason to use anonymous React component. This makes stack trace and React debugger pretty hard to use and is largely advise against by the React core team.

Thanks a lot @sapphi-red for all the triage you did last month, it was very helpful!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: hmr p3-minor-bug 🔨 An edge case that only affects very specific usage (priority)
Projects
None yet
10 participants