ArnaudBarre / eslint-plugin-react-refresh

Validate that your components can safely be updated with Fast Refresh
MIT License
221 stars 15 forks source link

Is `export default ` HOC pattern should be flagged? #6

Closed DianaSuvorova closed 1 year ago

DianaSuvorova commented 1 year ago

We get this type of pattern flagged by the rule:

  const MainPageHeader = () => <HeaderWrapper/>
  export default withRouter(MainPageHeader);

error:

AssertionError [ERR_ASSERTION]: Should have no errors but had 1: [
  {
    ruleId: 'only-export-components',
    severity: 1,
    message: 'Fast refresh only works when a file only export components. Move your component(s) to a separate file.',
    line: 2,
    column: 15,
    nodeType: 'Identifier',
    messageId: 'localComponents',
    endLine: 2,
    endColumn: 29
  }
]

while this one is valid from a react-refresh perspective:

      const MainPageHeader = () => <HeaderWrapper/>
      export const SubScreen =  withRouter(MainPageHeader);
ArnaudBarre commented 1 year ago

I think this is expected because otherwise you get an anonymous component. And it's not supported because it's bad for stacktrace/devtools tree exploration

If you're doing a massive refactor for that, I highly recommend you to stop the patten of default export, it's verbose, make renaming more complex, easily breaks grep search, invite people to only export one component per file, ...

DianaSuvorova commented 1 year ago

yes, I agree with all of the above.

I am still trying to figure out whether by react-refresh spec it triggers a whole tree reload:

https://github.com/pmmmwh/react-refresh-webpack-plugin/issues/99#issuecomment-634143932

https://github.com/facebook/react/issues/17142#issuecomment-543996489

DianaSuvorova commented 1 year ago

Ok, I think this is the most official answer I found:

If you write "export default () => { ... }" to declare components, they:

- will show up as Anonymous in stack traces
- will show up as Unknown in DevTools
- won't be checked by React-specific lint rules
- won't work with some features like Fast Refresh

Give components names!

Do you think it is reasonable to add a more descriptive error for anonymous components?

ArnaudBarre commented 1 year ago

Yes absolutely you can send a PR!

DianaSuvorova commented 1 year ago

Default HOC exports are actually flagged correctly with the current version of the plugin and the error message is descriptive enough. Apologies for the confusion.

One pattern that we still use in our codebase and that wasn't flagged is a default CallExpression export: compose()(MainComponent)

Here is a PR to address that: https://github.com/ArnaudBarre/eslint-plugin-react-refresh/pull/7