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

False positive on export { X as default } #41

Closed
geoffreytools opened this issue May 9, 2024 · 4 comments
Closed

False positive on export { X as default } #41

geoffreytools opened this issue May 9, 2024 · 4 comments

Comments

@geoffreytools
Copy link

Hello,

The following code produces a warning:

export { App as default };

const App = () => <>Test</>;

warning Fast refresh only works when a file only exports components. Move your component(s) to a separate file react-refresh/only-export-components

I am using Vite with the default config and fast refresh is working properly:

[vite] hmr update /src/App.tsx (x3)

I can tell this syntax is not in your test suite, so it's certainly not a bug.

I noticed your plugin is not the only one to choke at this syntax. Storybook does not like it when I export a story's meta object this way. I wonder why that is.

I like to have my exports at the top of the file as well as to use the arrow notation. This syntax is the only valid way to do achieve it.

Thank you for your work !

@ArnaudBarre
Copy link
Owner

Thanks for the short test case. I fixed this special case.

Nonetheless I would encourage you to move you export to export const App = () =>, for most React files there is only one export anyway. Also default export tend can also hide usage of a component when using just a grep search and this kind of pattern can make refactoring/view usage more tedious for IDEs/editors.

@geoffreytools
Copy link
Author

Thanks for the advice.

Unless you see something I am missing, if you export a named function as default and you use this name throughout your codebase, I think all of these downsides disappear: auto-imports and auto-renaming work, and you can find your cubs with a global search. I imagine this could be enforced with a a lint rule. If there is a collision in a file you can write import { default as Bar }.

I like how my import statements look like when I export types alongside the main thing. It is true that I rarely export companion components alongside a main component but I sometimes do it when it exposes slots. If there are too many of them I can always move them to one or multiple files and use the barrel export pattern so as to not break users.

@ArnaudBarre
Copy link
Owner

The issue with default export and so default import is that you can do import Bar from "../components/Foo.tsx" and then looking for Foo with global search will miss this usage. (It will still work with "See usages" but sometime it's quicker to just use grep search)
Also probably that renaming Foo to Foo2 will keep the usage of Bar intact (at least in JetBrains IDEs)

@geoffreytools
Copy link
Author

Sure but that would be breaking the rule of exporting a named function and using this name throughout your codebase. Moreover, you can also rename a named import. Then, global search would not work and re-factoring the original function would not change the local name either. The 2 systems really behave exactly the same.

I would insist on the syntax { default as Bar } in case of renaming though, because otherwise you could miss that something is being renamed.

Actually, the more I talk about it the more I want to write that lint rule.

Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants