feat(lint): add rule useComponentExportOnlyModules

[GunseiKPaseri]

Summary

Implement useComponentsOnlyModule( inspired eslint-plugin-react-refresh)

Closes #3560

This rule applies only to .jsx files. (Since .js files are also treated as .jsx, it is necessary to check the file extension.)
It scans the declarations in the file and enumerates the following:

• Component declarations that are not exported
• Exported components
• Exported non-components

The determination of whether something is a component or not is done simply by checking if the function or variable name is in PascalCase. (Exceptionally, cases where standard React functions like memo are used are also considered as components.)
The enumeration of exports is achieved by searching for JsExport and using a newly implemented utility (get_exported_items).
This utility extracts the names of declarations and the expressions being exported within JsExport. The declaration names are used to determine whether they are components, and the expressions are used for exception handling specific to React.

Based on the count of each of these, it outputs appropriate errors to ensure that the condition "components must always be exported, and only components should be exported" is met.

When attempting to export both components and non-components simultaneously, it instructs to move the export of non-components to a separate file.
If there are components that are not exported, it instructs to move the component to a new file (with a slightly different error message depending on whether there are other exported components).
No error is output in other cases.
Options have been implemented to ignore the export of constants (allowConstantExport), to treat specific non-PascalCase exports as components (allowExportNames), and to enforce the check on .js files as well (checkJS).
checkJS has not been implemented.

Test Plan

• The added tests function correctly

[codspeed-hq]

CodSpeed Performance Report

Merging #3576 will degrade performances by 7.96%

_{Comparing GunseiKPaseri:for-react-refresh (e3e6881) with main (0a80daa)}

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 105 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	GunseiKPaseri:for-react-refresh	Change
❌	js_analyzer[index_3894593175024091846.js]	37.3 ms	40.5 ms	-7.96%
⚡	db_17847247775464589309.json[cached]	14.2 ms	12.8 ms	+10.52%

[chansuke]

Like useNamingConvention, we can emphasize PascalCase and camelCase and I believe that this is valuable for the user.

ref: https://github.com/biomejs/biome/blob/main/crates/biome_js_analyze/src/lint/style/use_naming_convention.rs#L51-L54

[chansuke]

Let's add some information for HMR like below.

Hot Mudule Replacement(HMR)

[chansuke]

Maybe you can move the is_default function under the UseComponentsOnlyModuleOptions class to make it clear that this is for the field attribute, IMHO.

[GunseiKPaseri]

I understand that it should be that way. I wrote this based on biome_analyze/CONTRIBUTING.md and use_filenaming_convention.rs as references. Those might need to be revised as well. (For now, I deleted it since it doesn't seem like a crucial item.)

[chansuke]

let's use as_ref to avoid redundant clone().

if !exported_component_ids.is_empty() {
    return exported_non_component_ids
        .iter()
        .filter_map(|id| {
            id.identifier.as_ref().map(|identifier| {
                UseComponentsOnlyModuleState {
                    error: ErrorType::ExportedNonComponentWithComponent,
                    range: identifier.range(),
                }
            })
        })
        .collect();
}

[chansuke]

This is my preference, but I think naming it fn_name or function_name would be better maybe?

[chansuke]

From the user's point of view, adding this consideration is more useful, I believe.

               .note(markup! {
                    "Consider separating non-component exports into a new file." // Please update the description
                }),

[GunseiKPaseri]

That was originally written in the note section, but I addressed it by splitting the long sentence for clarity.👍

[ematipico]

I left a preliminary review. Can I ask you to update the PR description and explain the business logic you implemented for the rule? Try to be as much as technical as you want. This will help us to review your code, and provide better suggestions to prevent performance regressions

[ematipico]

When creating public APIs, we usually add doc tests.

biome/crates/biome_js_syntax/src/directive_ext.rs

Lines 6 to 22 in 4880f84

	/// Get the inner text of a string not including the quotes
	///
	/// ## Examples
	///
	/// ```
	/// use biome_js_factory::syntax::{JsDirective, JsSyntaxKind::*};
	/// use biome_js_factory::JsSyntaxTreeBuilder;
	/// use biome_rowan::AstNode;
	/// let mut tree_builder = JsSyntaxTreeBuilder::new();
	/// tree_builder.start_node(JS_DIRECTIVE);
	/// tree_builder.token(JS_STRING_LITERAL, "\"use strict\"");
	/// tree_builder.finish_node();
	/// let node = tree_builder.finish();
	/// let js_directive = JsDirective::cast(node).unwrap();
	/// let text = js_directive.inner_string_text().unwrap();
	/// assert_eq!(text, "use strict")
	/// ```

Also, think the doc of this function is incorrect. It doesn't return any pairs, it seems. ExportedItem has three fields

[ematipico]

We should document this type and its fields

[ematipico]

We should add some examples

[ematipico]

The first line of the docs is usually a small description of the rule. It's usually added in this table https://biomejs.dev/linter/rules/#accessibility

So the phrase could start with something like "Enforce React components ..." Or "Promotes the use of .."

[ematipico]

We should provide a link to Fash Refresh

[ematipico]

The name is weird, but I don't have better ideas 😅

[GunseiKPaseri]

How about useModuleComponentExportsOnly?

[chansuke]

This is hard...how about useComponentExportOnlyModules (This is just a suggestion).

[ematipico]

We should document these options. These options will be described inside our configuration schema, and users can read its docs.

[Conaclos]

Is there a way of reducing the number of options? For example is checkJs really useful? or even the two ther options?

[ematipico]

We have an API that returns the file source, you can use that:

let file_source = ctx.source_type::<JsFileSource>();

JsFileSource has methods to tell you which kind of file you're processing.

[GunseiKPaseri]

Due to relying on PascalCase for component detection, unrelated .js files that export in PascalCase for some reason may cause malfunction. We implement this exception handling to reduce the impact, but is_jsx() may return true for general JavaScript as well.

[ematipico]

but is_jsx() may return true for general JavaScript as well.

At the moment, no. Biome doesn't allow JSX inside .js files.

[GunseiKPaseri]

JS files within the test directory are tested as JSX even if they do not contain JSX syntax. Is this specific to the testing environment?

biome/crates/biome_js_analyze/tests/specs/nursery/useComponentsOnlyModule/invalid_component_and_constant_js_with_option.js.snap

Lines 15 to 28 in 8343762

	invalid_component_and_constant_js_with_option.js:1:14 lint/nursery/useComponentsOnlyModule ━━━━━━━━━━
	! Exporting a non-component with components is not allowed.
	> 1 │ export const CONSTANT = 3
	│ ^^^^^^^^
	2 │ export const Foo = () => {}
	3 │
	i Fast Refresh only works when a file only exports components.
	i Consider separating non-component exports into a new file.
	i If it is a component, it may not be following the variable naming conventions.

(The above snapshot shows the expected behavior when checkJS is set to true and targets JavaScript. Normally, I do not want any errors to be generated.)

I do not want to generate errors like the ones mentioned above for .js files. But ctx.source_type::<JsFileSource>().is_jsx() returned true.

[ematipico]

I have to disregard my previous statement, I am sorry. It seems that our parser enables JSX parsing by default for .js files :(

So yeah, you better keep this check for now. I believe we should break this behaviour in v2.0

[ematipico]

Since ExportedItem is a public API, it makes more sense to expose a function called exports_binding() (just a suggestion, if you have a better name, that's great), and pass the binding name.

[chansuke]

@GunseiKPaseri
Thank you for your update! I will take a look tonight.

[chansuke]

This is hard...how about useComponentExportOnlyModules (This is just a suggestion).

Took very long, didn't review. They can follow up if they want