Add noDirectMutationState rule and tests #582

stefanuros · 2020-06-05T18:29:14Z

#341
Implemented react/no-direct-mutation-state.

If anyone has any ideas for scenarios that should be tested, please let me know. This was my first contribution so any advice/suggestions about the code are welcome.

stefanuros · 2020-06-05T23:52:52Z

Currently unable to continue as this error is coming up when I run scripts/dev-rome lint. I am working to fix this and once it is, I will make the PR ready for review.

Unhandled CLI query error
Error: raw and highlighted line count mismatch 841 !== 966
  at ___R$project$rome$$romejs$cli$diagnostics$utils_ts$toLines (project-rome/@romejs/cli-diagnostics/utils.ts:80:8)
  at ___R$project$rome$$romejs$cli$diagnostics$DiagnosticsPrinter_ts$default.addFileSource (project-rome/@romejs/cli-diagnostics/DiagnosticsPrinter.ts:241:12)
  at ___R$project$rome$$romejs$cli$diagnostics$DiagnosticsPrinter_ts$default.fetchFileSources (project-rome/@romejs/cli-diagnostics/DiagnosticsPrinter.ts:344:9)
  at ___R$project$rome$$romejs$cli$diagnostics$DiagnosticsPrinter_ts$default.print (project-rome/@romejs/cli-diagnostics/DiagnosticsPrinter.ts:351:7)
  at ___R$project$rome$$romejs$core$master$Master_ts$default.handleRequestError (project-rome/@romejs/core/master/Master.ts:961:12)
  at ___R$project$rome$$romejs$core$master$Master_ts$default.dispatchRequest (project-rome/@romejs/core/master/Master.ts:888:51)
  at global.runMicrotasks
  at process.processTicksAndRejections (internal/process/task_queues.js:97:4) generated source location

sebmck · 2020-06-05T23:54:19Z

Can you rebase and try again? #583 was merged that should resolve that error however it will uncover another that might be more actionable.

stefanuros · 2020-06-06T00:01:07Z

I tried that, unfortunately no change. Its something to do with the noDirectMutationState.ts file. If I remove most of the file, it stops giving me that error. By most of the file, I mean leave just the barebones. So something in the file is causing the issue.

stefanuros · 2020-06-06T05:56:20Z

@sebmck I think it has something to do with doesNodeMatchPattern(node.left, "this.state") on line 27 of noDirectMutationState.ts. When I removed that statement, running scripts/dev-rome lint works fine. Perhaps I am misunderstanding about how to use doesNodeMatchPattern or what JSAssignmentExpression contains?

macovedj · 2020-06-06T19:45:37Z

Did some looking, had some thoughts. fwiw if I just pull your branch I don't get the error.
Also I think your implementation takes issue with general mutation of state in a class, not necessarily in a react component. When you run the lint rule, it thinks that rome itself is in violation, even though rome isn't written in react.

stefanuros · 2020-06-07T01:03:58Z

The new commit (b2206e0) makes sure the node being considered is a react component. React components are defined by a class extending React.Component, a class extending Component, or being withing a function call where the identifier is createReactClass. This new change has removed the raw vs highlighted issue I was having (still unsure what was causing it). If I am missing any tests I should be checking, or any different ways of defining react components, please let me know.

VictorHom

Thanks for the pr. I left a comment on removing the createReactClass test cases. That might simplify some checks in packages/@romejs/compiler/lint/rules/react/noDirectMutationState.ts

VictorHom · 2020-06-09T03:36:15Z

packages/@romejs/diagnostics/descriptions/lint.ts

+			{
+				type: "log",
+				category: "info",
+				text: "Calling <emphasis>setState()</emphasis> after mutating <emphasis>this.state</emphasis> directly may replace the mutation you made.",


I think adding an additional "The only place to set this.setState directly is in the constructor of a react class component." is helpful.

VictorHom · 2020-06-09T03:40:40Z

packages/@romejs/compiler/lint/rules/react/noDirectMutationState.test.ts

+			{
+				invalid: [
+					`
+            var Hello = createReactClass({


awesome on the extensive test cases! from a decision in previous discussions (#341 (comment) as one place), you don't need to account for the createReactClass case. Sorry for the noise

Whoops I missed that. Ill take those checks out. Ive changed the createReactClass tests as well and stripped a couple other repetitive ones.

ematipico

WOW! Amazing PR with amazing comments!! Top notch!!!

VictorHom

One small change requested.

When you get a chance to also squash your commits, that would be great

VictorHom · 2020-06-10T12:31:00Z

packages/@romejs/compiler/lint/rules/react/noDirectMutationState.test.md

+    5 │               return <div>Hello {this.props.name}</div>;
+    6 │             }
+
+  ℹ Calling setState() after mutating this.state directly may replace the mutation you made. Theonly


this isn't from your changes, but I'm just noting how The only turned in Theonly. I'll make an issue after when I'm more clear on what is happening.

I tried something out and made this the new message

Calling <emphasis>setState()</emphasis> The only after mutating <emphasis>this.state</emphasis> directly may replace the mutation you made. The only place you may set <emphasis>this.state</emphasis> directly is in a constructor of a react class component.

I added The only after the first emphasis tag. This is the result of that change.

ℹ Calling setState() The only after mutating this.state directly may replace the mutation youmade. The only place you may set this.state directly is in a constructor of a react class component.

Its related to the last 2 words in that line being combined together.

It tries to merge the last 2 words if the line is 97 characters long. If its 98 or more, it doesn't try it, but if it is exactly 97, it removes 1 space to shrink it to 96 characters. Is that length significant?
Here is another example showing that.
lint.ts

aCalling <emphasis>setState()</emphasis> after mutating <emphasis>this.state</emphasis> directly may replace the mutation you made. abc abc ab The only place you may set <emphasis>this.state</emphasis> directly is in a constructor of a react class component.

test.md

ℹ aCalling setState() after mutating this.state directly may replace the mutation you made. abcabc ab The only place you may set this.state directly is in a constructor of a react classcomponent.

Notice the padding I added at the beginning of the lines and notice the abc abc being merged in beginning of the second sentence, and the class component being merged at the end of it. If I took out the letter a at the beginning of each line, the abc abc and class component would not be merged.

VictorHom · 2020-06-10T12:35:19Z

packages/@romejs/compiler/lint/rules/react/noDirectMutationState.ts

+	// Check if it extends React.Component or Component
+		node.type === "JSClassHead" &&
+		node.superClass !== undefined &&
+		(doesNodeMatchPattern(node.superClass, "React.Component") ||


Another component that is good to check is React.PureComponent

Formatting changes Included check for react components only createReactClass is not being tested anymore

VictorHom

Thanks for looking into the error message and also this pr! this lgtm 👍

stefanuros · 2020-06-11T04:38:23Z

@VictorHom Awesome, thanks for your help!

stefanuros marked this pull request as draft June 5, 2020 18:31

stefanuros force-pushed the noDirectMutationState branch 2 times, most recently from ccfc975 to cf5ece1 Compare June 5, 2020 21:43

stefanuros mentioned this pull request Jun 5, 2020

WIP: No direct mutation state #395

Closed

stefanuros force-pushed the noDirectMutationState branch 2 times, most recently from b35805f to 3c8d797 Compare June 7, 2020 00:58

stefanuros force-pushed the noDirectMutationState branch from 3c8d797 to b2206e0 Compare June 7, 2020 01:06

stefanuros marked this pull request as ready for review June 7, 2020 01:07

stefanuros mentioned this pull request Jun 7, 2020

Lint rule react/no-direct-mutation-state; Uncertainties about call stacks and functional components #588

Closed

VictorHom suggested changes Jun 9, 2020

View reviewed changes

ematipico approved these changes Jun 9, 2020

View reviewed changes

stefanuros force-pushed the noDirectMutationState branch 2 times, most recently from f9e199a to 5bc25e4 Compare June 9, 2020 18:47

stefanuros requested a review from VictorHom June 9, 2020 18:47

stefanuros force-pushed the noDirectMutationState branch from 5bc25e4 to 24def7e Compare June 9, 2020 18:53

VictorHom suggested changes Jun 10, 2020

View reviewed changes

Add noDirectMutationState rule and tests

995bfd4

Formatting changes Included check for react components only createReactClass is not being tested anymore

stefanuros force-pushed the noDirectMutationState branch from 24def7e to 995bfd4 Compare June 10, 2020 17:55

stefanuros requested a review from VictorHom June 10, 2020 18:01

VictorHom approved these changes Jun 11, 2020

View reviewed changes

sebmck changed the base branch from master to main June 12, 2020 03:57

VictorHom merged commit bd6048f into rome:main Jun 12, 2020

stefanuros deleted the noDirectMutationState branch June 22, 2020 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add noDirectMutationState rule and tests #582

Add noDirectMutationState rule and tests #582

stefanuros commented Jun 5, 2020

stefanuros commented Jun 5, 2020

sebmck commented Jun 5, 2020

stefanuros commented Jun 6, 2020 •

edited

Loading

stefanuros commented Jun 6, 2020 •

edited

Loading

macovedj commented Jun 6, 2020

stefanuros commented Jun 7, 2020 •

edited

Loading

VictorHom left a comment

VictorHom Jun 9, 2020 •

edited

Loading

VictorHom Jun 9, 2020

stefanuros Jun 9, 2020

ematipico left a comment

VictorHom left a comment

VictorHom Jun 10, 2020

stefanuros Jun 10, 2020 •

edited

Loading

stefanuros Jun 10, 2020

VictorHom Jun 10, 2020

VictorHom left a comment •

edited

Loading

stefanuros commented Jun 11, 2020

Add noDirectMutationState rule and tests #582

Add noDirectMutationState rule and tests #582

Conversation

stefanuros commented Jun 5, 2020

stefanuros commented Jun 5, 2020

sebmck commented Jun 5, 2020

stefanuros commented Jun 6, 2020 • edited Loading

stefanuros commented Jun 6, 2020 • edited Loading

macovedj commented Jun 6, 2020

stefanuros commented Jun 7, 2020 • edited Loading

VictorHom left a comment

Choose a reason for hiding this comment

VictorHom Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

VictorHom Jun 9, 2020

Choose a reason for hiding this comment

stefanuros Jun 9, 2020

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

VictorHom left a comment

Choose a reason for hiding this comment

VictorHom Jun 10, 2020

Choose a reason for hiding this comment

stefanuros Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

stefanuros Jun 10, 2020

Choose a reason for hiding this comment

VictorHom Jun 10, 2020

Choose a reason for hiding this comment

VictorHom left a comment • edited Loading

Choose a reason for hiding this comment

stefanuros commented Jun 11, 2020

stefanuros commented Jun 6, 2020 •

edited

Loading

stefanuros commented Jun 6, 2020 •

edited

Loading

stefanuros commented Jun 7, 2020 •

edited

Loading

VictorHom Jun 9, 2020 •

edited

Loading

stefanuros Jun 10, 2020 •

edited

Loading

VictorHom left a comment •

edited

Loading