ArnaudBarre / eslint-plugin-react-refresh

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

Warn exports instead of components when using only components #33

Open skovhus opened 11 months ago

skovhus commented 11 months ago

It seems like object export isn't correctly handled. In the following simplified example, NameInput has a warning, but it should actually be SomeInternalInput and SomeInternalForm OR TitleSection that had an error.

const SomeInternalInput = () => <div><NameInput /></div>
const SomeInternalForm = () => <div />

// Shows a warning, even though it isn't exported
const NameInput = styled(Input)`
  font-weight: 500;
`;

export const TitleSection = {
  Input: SomeInternalInput,
  Form: SomeInternalForm,
};
ArnaudBarre commented 11 months ago

I can try to better report errors, but this case is justified and the error is always more for the file than for a specific export.

This file can't be correctly Fast Refresh sadly (this technically could be should have support from the runtime to check that the object only export components et exactly the same keys than in the previous update, which not a one line simple update)

skovhus commented 11 months ago

Thanks. Yeah I understand that it isn't supported by fast refresh, but it would be great to improve the warning placement to the actual variables that are exported.

ArnaudBarre commented 11 months ago

So actually I've look at it and it correctly warns on all components that could loose state on Fast Refresh. The action is too split the components from the export, sometimes it would mean moving the component to another file because it makes sense to export an object containing a component in that file, sometimes this would mean moving the export to another file because it's mostly an helper. I think the current behaviour is ok but I'm open to changing the warning message to something else that would make it more clear when on the export side