Rel1cx / eslint-react

A series of composable ESLint rules for libraries and frameworks that use React as a UI runtime.
https://eslint-react.xyz
MIT License
156 stars 6 forks source link

[docs] Inconsistency and Potential Breaking Change in Documentation for `no-useless-fragment` Rule #599

Closed liby closed 3 weeks ago

liby commented 3 weeks ago

Issue Description

I am currently migrating from eslint-plugin-react to eslint-react and have observed potential inconsistencies and incorrect examples in the documentation for the no-useless-fragment rule. Below are the details along with concerns about a possible change to the rule's behavior.

Documentation Comparison

Issue

In practice, neither eslint-plugin-react nor eslint-react currently flags the code return <Fragment>foo</Fragment>; as an error. This is consistent with other examples in the eslint-plugin-react documentation where similar uses of fragments are deemed correct, such as:

const cat = <>meow</>

This suggests that users are allowed to decide whether to return a string directly or wrapped in a JSX element without triggering the no-useless-fragment rule.

Concerns About Rule Modification

  1. Potential Breaking Change: Modifying the rule to make the current examples trigger an error could introduce a breaking change for users migrating from eslint-plugin-react. This might lead to unexpected behavior and frustration among the user base.

  2. Limiting User Flexibility: Forcing a change in this rule would prevent users from returning a single child as a JSX.Element when they deem it necessary. This could limit flexibility in coding styles and practices.

Suggested Action

I believe the documentation might need a thorough review and possible correction to accurately reflect the actual behavior of the rule and avoid any unintended breaking changes. It would be beneficial to align the rule's behavior with its documentation or clarify the intended use cases more clearly.

Rel1cx commented 3 weeks ago

Thanks for the feedback on the issue and I agree that the documentation needs a thorough review and possible corrections.

SukkaW commented 3 weeks ago

2. Limiting User Flexibility: Forcing a change in this rule would prevent users from returning a single child as a JSX.Element when they deem it necessary. This could limit flexibility in coding styles and practices.

It is the limitation of TypeScript, and since TypeScript 5.1, TypeScript now allows a component to return React.ReactNode instead of React.ReactElement. The React always allows a component to render React.ReactNode, so I don't see any reason why <>{single child}</> is needed after TypeScript 5.1.

Rel1cx commented 3 weeks ago
  1. Limiting User Flexibility: Forcing a change in this rule would prevent users from returning a single child as a JSX.Element when they deem it necessary. This could limit flexibility in coding styles and practices.

It is the limitation of TypeScript, and since TypeScript 5.1, TypeScript now allows a component to return React.ReactNode instead of React.ReactElement. The React always allows a component to render React.ReactNode, so I don't see any reason why <>{single child}</> is needed after TypeScript 5.1.

image https://snyk.io/advisor/npm-package/typescript Considering that typescript 4.9.5 still gets 9.4M weekly downloads, let's keep the current behavior of this rule for now.

liby commented 3 weeks ago
  1. Limiting User Flexibility: Forcing a change in this rule would prevent users from returning a single child as a JSX.Element when they deem it necessary. This could limit flexibility in coding styles and practices.

It is the limitation of TypeScript, and since TypeScript 5.1, TypeScript now allows a component to return React.ReactNode instead of React.ReactElement. The React always allows a component to render React.ReactNode, so I don't see any reason why <>{single child}</> is needed after TypeScript 5.1.

I'd like to highlight three main points in response:

  1. Focus on Documentation Accuracy: The primary concern raised in this issue is the accuracy of the documentation. It presents examples of code that would supposedly trigger errors when this rule is enabled, yet these codes do not actually result in errors in real scenarios. This leads to the following question: Should we update the documentation or change the code behavior to address this discrepancy? Either approach could resolve the issue, but which one is more appropriate?

  2. Assumptions About User Environment and Knowledge: We should not assume the level of knowledge or the specifics of the development environment of all users. Many may not be aware of this context. Changing the code to make return <Fragment>foo</Fragment> result in an error could confuse users migrating from eslint-plugin-react, even if we thoroughly document this change. The unexpected behavior could lead to significant frustration.

  3. Existing User Habits and TypeScript Allowances: Given the prevalence of eslint-plugin-react, we must acknowledge that some users have grown accustomed to certain behaviors. Even though TypeScript 5.1 now allows a broader range of return types, insisting that users change from return <>114514</> to return 114514 seems peculiar. Moreover, many users may still be using versions prior to TypeScript 5.1.