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

📎 Implement eslint-plugin-react-refresh #3560

Closed
1 task done
GunseiKPaseri opened this issue Aug 1, 2024 · 5 comments · Fixed by #3576
Closed
1 task done

📎 Implement eslint-plugin-react-refresh #3560

GunseiKPaseri opened this issue Aug 1, 2024 · 5 comments · Fixed by #3576
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Feature Status: new feature to implement

Comments

@GunseiKPaseri
Copy link
Contributor

GunseiKPaseri commented Aug 1, 2024

Description

from #3 (reply in thread)

react-refresh

react-refresh is an official package of react that provides bundler with a Fast Refresh feature that allows real-time updates of running React apps.
In addition to react-native, next.js, @vitejs/plugin-react and @parcel/runtime-react-refresh are often used behind the scenes in development environments.
It is a so-called hot reload, but it is characterized by the fact that only edited components can be updated minute by minute while maintaining the state.
For more detailed information, please refer to the documentation of each bundler.

eslint-plugin-react-refresh

eslint-plugin-react-refresh is a plugin also linked from @vitejs/plugin-react that defines rules to make react-refresh function properly.

Rule react-refresh/only-export-components is too large.

Already implemented

I plan to implement it.

The following are not included in the current lint rules, but they are desired for future implementation.

more info: #2548

@ematipico ematipico added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Feature Status: new feature to implement labels Aug 1, 2024
@GunseiKPaseri
Copy link
Contributor Author

GunseiKPaseri commented Aug 2, 2024

  • The main lint rule also outputs errors equivalent to noReExportAll. What would be an appropriate way to guide the enabling of this during migration?

  • For this extension, we want to apply it only to .jsx and .tsx files. Since it seems that this cannot be achieved by specifying language: "jsx", is it appropriate to implement this check within the run function?

  • There is a function that we want to share with other nursery rules (noYodaExpression). Where would be the appropriate place to extract this function?

@arendjr
Copy link
Contributor

arendjr commented Aug 2, 2024

For this extension, we want to apply it only to .jsx and .tsx files

Note that while TypeScript is strict about requiring a .tsx extension for files with JSX syntax, such a requirement doesn't exist for JavaScript. A .js file with JSX syntax can be totally fine depending on one's bundler configuration.

@Conaclos
Copy link
Member

Conaclos commented Aug 3, 2024

The main lint rule also outputs errors equivalent to noReExportAll. What would be an appropriate way to guide the enabling of this during migration?

Unfortunately Biome migration tool doesn't support a source to be associated to several Biome rules. Thus, for now, it is not possible to do that.

For this extension, we want to apply it only to .jsx and .tsx files. Since it seems that this cannot be achieved by specifying language: "jsx", is it appropriate to implement this check within the run function?

Could you explain the reasoning behind this choice?
If it is really necessary, you can retrieve the file path (something like ctx.file_path()) and check the extension with the standard library.

There is a function that we want to share with other nursery rules (noYodaExpression). Where would be the appropriate place to extract this function?

If the function is quite specific, you can just make it public in one of the rule (pub(crate)) and import it in the other.
If the function is quite generic and seems useful in a general way, then you can place it in the biome_js_syntax crate.
We have a bunch of files ending with _ext where it could be placed.

In addition, since the Pascal case is used to determine whether a component is a component or not, we plan to implement functions related to the constraints around it.

biome_string_case::Case::identify should be helpful in that regard :)

@GunseiKPaseri
Copy link
Contributor Author

Unfortunately Biome migration tool doesn't support a source to be associated to several Biome rules. Thus, for now, it is not possible to do that.

mmm...

Could you explain the reasoning behind this choice?
If it is really necessary, you can retrieve the file path (something like ctx.file_path()) and check the extension with the standard library.

In detecting React components, only the name is used, which can lead to false positives. The original lint rule was implemented to avoid this issue.

@Conaclos
Copy link
Member

Conaclos commented Aug 8, 2024

Unfortunately Biome migration tool doesn't support a source to be associated to several Biome rules. Thus, for now, it is not possible to do that.

Although, it could be fairly easy to change how we generate migrate and website pages to allow this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Feature Status: new feature to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants