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

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

Merged

Conversation

DouglasDev
Copy link
Contributor

@DouglasDev DouglasDev commented Jul 2, 2022

Description

This addresses vitejs/vite-plugin-react#8 especially: vitejs/vite-plugin-react#8

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

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

@netlify
Copy link

netlify bot commented Jul 2, 2022

Deploy Preview for vite-docs-main canceled.

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

@DouglasDev DouglasDev changed the title fix: don't count class declarations as react fast refresh boundry (close #3675) fix: don't count class declarations as react fast refresh boundry (fixes #3675) Jul 2, 2022
@sapphi-red sapphi-red added plugin: react p3-minor-bug 🔨 An edge case that only affects very specific usage (priority) labels Jul 3, 2022
@DouglasDev
Copy link
Contributor Author

DouglasDev commented Jul 11, 2022

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.

@sapphi-red sapphi-red added p4-important Violate documented behavior or significantly improves performance (priority) pending triage and removed p3-minor-bug 🔨 An edge case that only affects very specific usage (priority) p4-important Violate documented behavior or significantly improves performance (priority) labels Jul 14, 2022
@sapphi-red
Copy link
Member

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>
  }
}

@DouglasDev
Copy link
Contributor Author

DouglasDev commented Jul 16, 2022

@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.

@sapphi-red
Copy link
Member

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.

if (useFastRefresh && /\$RefreshReg\$\(/.test(code)) {
const accept = isReasonReact || isRefreshBoundary(result.ast!)
code = addRefreshWrapper(code, id, accept)
}

@sapphi-red sapphi-red added p3-minor-bug 🔨 An edge case that only affects very specific usage (priority) and removed pending triage labels Jul 30, 2022
@patak-dev patak-dev merged commit 5a18284 into vitejs:main Aug 10, 2022
24 checks passed
@DouglasDev
Copy link
Contributor Author

@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".

@sapphi-red
Copy link
Member

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>
  }
}

@DouglasDev
Copy link
Contributor Author

@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.

@sapphi-red
Copy link
Member

@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?

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

Successfully merging this pull request may close these issues.

None yet

3 participants