+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyzer): code action for noVoidElementsWithChildren #3286

Merged
merged 4 commits into from
Sep 30, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Sep 27, 2022

Summary

Closes #3272

The PR implements the code action for the rule noVoidElementsWithChildren

Test Plan

Updated the test cases and added new test case for the documentation

@ematipico ematipico requested a review from a team September 27, 2022 17:34
@netlify
Copy link

netlify bot commented Sep 27, 2022

Deploy Preview for rometools ready!

Name Link
🔨 Latest commit 1f39c0f
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6336e74c2fbfe70008e8e540
😎 Deploy Preview https://deploy-preview-3286--rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ematipico ematipico temporarily deployed to netlify-playground September 27, 2022 17:34 Inactive
@ematipico ematipico changed the title feat(rome_js_analyzer): implement code action for `noVoidElementsWith… feat(rome_js_analyzer): code action for noVoidElementsWithChildren Sep 27, 2022
@github-actions
Copy link

github-actions bot commented Sep 27, 2022

Comment on lines 70 to 77
/// If the current element has children props in style `<Component>'children'</Component>
children_cause: bool,
/// If the current element has the prop `dangerouslySetInnerHTML`
dangerous_prop_case: bool,
dangerous_prop_case: Option<UnwelcomedProp>,
/// If the current element has the prop `children`
children_prop: Option<UnwelcomedProp>,
/// An instance of [ReactCreateElementCall]
create_react_element: Option<ReactCreateElementCall>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have all combinations of children_cause, dangerous_prop_case, children_prop, and crate_react_element` or can we limit the possible combinations and represent them as a union?

I'm asking because limiting the possibilities can help readers to understand what combinations can exist and using an enum to name them makes them into explicit concepts (that can even be documented). It may also help to avoid some unreachable branches in the code.

Copy link
Contributor Author

@ematipico ematipico Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really depends on how we want the rule to work. My initial implementation was using an enum, but then I noticed that the old rule works by tracking all the invalid cases.

In this rule, we could have up to three errors at once:

React.createElement('img', {
	children: 'child', // <= diagnostic triggered
	dangerouslySetInnerHTML: 'html' // <= diagnostic triggered
}, 'child') // <= diagnostic triggered for 'child'

The upside of using an enum state would make the rule simple to understand for us, on the other hand this would generate up to three diagnostics that will show up to three code actions - basically we shift the burden to the user.

Tracking all the states at once allows us to emit one single code action and one single diagnostics.

Eventually ALL the suggested fixes are applied and the resultant code will be the same in both cases.

I don't mind both approaches, but I am more keen to use the single diagnostic approach.

@ematipico ematipico temporarily deployed to netlify-playground September 28, 2022 07:41 Inactive
@ematipico ematipico force-pushed the feature/no-void-elements-code-action branch from 55473cf to 670b855 Compare September 28, 2022 08:44
@ematipico ematipico temporarily deployed to netlify-playground September 28, 2022 08:44 Inactive
@ematipico ematipico added the A-Linter Area: linter label Sep 30, 2022
@ematipico ematipico force-pushed the feature/no-void-elements-code-action branch from 9fc9658 to 1f39c0f Compare September 30, 2022 12:55
@ematipico ematipico temporarily deployed to netlify-playground September 30, 2022 12:55 Inactive
@ematipico ematipico merged commit e4340a4 into main Sep 30, 2022
@ematipico ematipico deleted the feature/no-void-elements-code-action branch September 30, 2022 13:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

noVoidElementsWithChildren
2 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载