vite icon indicating copy to clipboard operation
vite copied to clipboard

fix: don't count class declarations as react fast refresh boundry (fixes #3675)

Open DouglasDev opened this issue 3 years ago • 5 comments

Description

This addresses https://github.com/vitejs/vite/issues/3675 especially: https://github.com/vitejs/vite/issues/3675#issuecomment-993346896

If an export in a Vite React project is a React class component, editing the contents of the components and then saving the file will not cause any updates in the browser during development (neither a fast refresh nor a full refresh). I fixed this by checking for class components in the isRefreshBoundary and returning false if one is found.

Additional context

I'm pretty sure this will work but I wasn't able to test locally. I tried adding the follow to my package.json in a test React project:

"pnpm": {
    "overrides": {
      "vite": "link:../vite/packages/vite",
      "@vitejs/plugin-react": "link:../vite/packages/plugin-react"
    }
  }

however the changes in the plugin-react didn't seem to be reflected in my project.

If we could merge this into v2 of vite as well, that would be appreciated since this bug is creating a worse dev experience for many projects on v2.


What is the purpose of this pull request?

  • [x] Bug fix
  • [ ] New Feature
  • [ ] Documentation update
  • [ ] Other

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

  • [x] Read the Contributing Guidelines.
  • [x] Read the Pull Request Guidelines and follow the Commit Convention.
  • [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • [ ] Ideally, include relevant tests that fail without this PR but pass with it.

DouglasDev avatar Jul 02 '22 03:07 DouglasDev

Deploy Preview for vite-docs-main canceled.

Name Link
Latest commit c3ca293afd2c89897580c828b1dedeb9eae13864
Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62bfbd6c722d8f0008797e4b

netlify[bot] avatar Jul 02 '22 03:07 netlify[bot]

Personally, I wouldn't consider this a minor bug. Why? The beginning of the docs tell us:

Vite (French word for "quick", pronounced /vit/, like "veet") is a build tool that aims to provide a faster and leaner development experience for modern web projects

If HMR is broken so badly that I have to manually refresh the page each time I update a component, Vite can no longer be considered "quick" for development, and hence the entire point of using Vite is defeated and I might as well go back to using webpack.

DouglasDev avatar Jul 11 '22 16:07 DouglasDev

I was not able to reproduce. Editing class components caused a full reload properly for me. I tested the components below. Would you create a reproduction or add a test case?


import React from 'react'

function connect() {
  return function(component) {
    return component
  }
}

class ClassComponent extends React.Component {
  render() {
    return <p>Hello from class component.</p>
  }
}

export const WrappedClassComponent = connect()(ClassComponent)
import React from 'react'

class ClassComponent extends React.Component {
  render() {
    return <p>Hello from class component.</p>
  }
}

export default ClassComponent
import React from 'react'

export class ClassComponent extends React.Component {
  render() {
    return <p>Hello from class component.</p>
  }
}

sapphi-red avatar Jul 16 '22 14:07 sapphi-red

@sapphi-red

I was not able to reproduce. Editing class components caused a full reload properly for me. I tested the components below. Would you create a reproduction or add a test case?

import React from 'react'

function connect() {
  return function(component) {
    return component
  }
}

class ClassComponent extends React.Component {
  render() {
    return <p>Hello from class component.</p>
  }
}

export const WrappedClassComponent = connect()(ClassComponent)
import React from 'react'

class ClassComponent extends React.Component {
  render() {
    return <p>Hello from class component.</p>
  }
}

export default ClassComponent
import React from 'react'

export class ClassComponent extends React.Component {
  render() {
    return <p>Hello from class component.</p>
  }
}

Thanks for your reply. I created a minimal reproduction of the bug here: https://github.com/DouglasDev/vite-hmr-bug

to reproduce:

  1. clone the repo, install and run pnpm dev (I tested with pnpm) and load the app in chrome
  2. open src/App.jsx
  3. try editing the text where it says "Editing this text doesn't update" on line 39 and saving
  4. the browser will not update until you manually refresh the page.

Note: I think having both a functional and class component in the same file has something to do with the bug.

DouglasDev avatar Jul 16 '22 15:07 DouglasDev

Note: I think having both a functional and class component in the same file has something to do with the bug.

I think so. When a functional component exists (it does not need to be an exported one), /\$RefreshReg\$\(/.test(code) will be true. And when accept is true, import.meta.accept will be injected and a full-reload won't happen.

https://github.com/vitejs/vite/blob/e6f3b026bd1e281cfad6b19386195b6c2dc94165/packages/plugin-react/src/index.ts#L355-L358

sapphi-red avatar Jul 17 '22 18:07 sapphi-red

@patak-dev @sapphi-red I tried updating my packages to the latest version and I'm still seeing this bug occur. Did either of you test that my fix worked? I wasn't able to figure out how to test it myself (explained in the "Additional context"). I can make a second attempt at a fix if someone can walk me through how to run the @vitejs/plugin-react plugin in "development mode".

DouglasDev avatar Aug 15 '22 18:08 DouglasDev

I did test this. It seems it works for named exports, but not for default export.

export function App() {
  return <div className="App"></div>
}

export class App2 extends Component {
  constructor(props){
    super(props)
  }

  render(){
    return <div className="App"></div>
  }
}

sapphi-red avatar Aug 16 '22 04:08 sapphi-red

@sapphi-red If you can give me some guidance as to how to test the react plugin in development, I can try to write a fix.

DouglasDev avatar Aug 16 '22 23:08 DouglasDev

@DouglasDev

I'm pretty sure this will work but I wasn't able to test locally. I tried adding the follow to my package.json in a test React project:

I'm not sure why this is not working. Did you run pnpm build?

sapphi-red avatar Aug 18 '22 12:08 sapphi-red