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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New] Component detection: add util.isReactHookCall #3156

Merged

Conversation

duncanbeevers
Copy link
Contributor

@duncanbeevers duncanbeevers commented Dec 13, 2021

  • Rename Components test suite filename to match sibling lib/util/Components filename.
  • Extend Components testComponentsDetect function to accept custom instructions, and to accumulate the results of processing those instructions.
  • Add utility to check whether a CallExpression is a React hook call.

馃棧 Discussion

This work extracts the somewhat complicated guard clause from #2921
It also makes hooks-usage detection generic, so the util can be used to check for:

I converted this PR to a Draft while I work to extract this in a way that leaves the hook-use-state rule clean and easy to process.

Rename Components test suite filename to match sibling lib/util/Components filename.
Extend Components testComponentsDetect function to accept custom instructions, and to accumulate the results of processing those instructions.
Add utility to check whether a CallExpression is a React hook call.
@duncanbeevers duncanbeevers marked this pull request as draft December 13, 2021 14:43
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2021

Codecov Report

Merging #3156 (f06944d) into master (d56fdb8) will increase coverage by 0.00%.
The diff coverage is 98.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3156   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files         119      119           
  Lines        8130     8180   +50     
  Branches     2895     2934   +39     
=======================================
+ Hits         7928     7977   +49     
- Misses        202      203    +1     
Impacted Files Coverage 螖
lib/util/Components.js 98.85% <98.00%> (-0.10%) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update d56fdb8...f06944d. Read the comment docs.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2021

I renamed the test file in the default branch so when you rebase it'll show the diff properly :-)

@duncanbeevers duncanbeevers force-pushed the util-is-react-import-shadowed branch 2 times, most recently from 3ef8aba to b590a60 Compare December 13, 2021 18:22
lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
Comment on lines +31 to +36
const augmentedInstructions = fromEntries(
entries(instructions || {}).map((nodeTypeAndHandler) => {
const nodeType = nodeTypeAndHandler[0];
const handler = nodeTypeAndHandler[1];
return [nodeType, (node) => {
instructionResults.push({ type: nodeType, result: handler(node, context, components, util) });
Copy link
Member

Choose a reason for hiding this comment

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

i'm a bit confused about these changes; could you walk me through why they're needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of the Components test scaffolding is to allow exfiltration of the Components.detect locals to a scope in the test where assert is available, but without having to write a complete rule for every test with its own Program:exit clause.

My original implementation exfiltrated the Components.detect locals, but didn't make it easy to actually test some of those locals; in particular I couldn't test utils because the utility functions expect AST nodes, and by the time Program:exit fires, I've lost the CallExpression nodes I need.

These changes are intended to aid in writing new test cases for Components functionality; specifically, for testing utils functionality whose methods expect many different AST node types besides CallExpression.

Custom instructions passed into the testComponentsDetect get augmented and then incorporated into a custom rule, and when that rule runs over code the augmented instructions do two things:

  1. Provide the Components.detect locals to the instruction callback. Ordinarily the instruction callback only receives a node as an argument and relies on having been defined in the Components.detect closure to access the special locals it provides.
  2. Accumulate a log in instructionResults as the visitor traverses the parsed AST; the log tracks which node type was traversed, and the result of that visit.

Today

Taken together, these allow creation of test cases intended to read something like:

The visitor visited a CallExpression node
The visitor had access to the util local
The util function isReactHookCall returned the following result for the visited CallExpression node

Tomorrow

Down the road, as we work to flesh out coverage for the code in Components, we can write new tests cases.

The visitor visited a ClassDeclaration
The visitor had access to the util local
The util function isES6Component returned the following result for the visited ClassDeclaration node


return Object.assign({}, augmentedInstructions, {
'Program:exit'(node) {
if (augmentedInstructions['Program:exit']) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't yet exercised, but I figured if someone needed to test util functionality when handling the Program:exit instruction it should behave the same way as handling any other instruction type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a self-test section to the test/util/Components file in order to exercise the logging behavior while visiting Program:exit instructions.

@duncanbeevers duncanbeevers changed the title [Test] Component util isReactHookCall [Tests] Component util isReactHookCall Dec 14, 2021
@duncanbeevers duncanbeevers force-pushed the util-is-react-import-shadowed branch 2 times, most recently from e9dd937 to 2f212c6 Compare December 14, 2021 20:12
@duncanbeevers duncanbeevers marked this pull request as ready for review December 15, 2021 16:39
@ljharb ljharb changed the title [Tests] Component util isReactHookCall [New] Component detection: add util.isReactHookCall Dec 22, 2021
@ljharb ljharb force-pushed the util-is-react-import-shadowed branch from f06944d to 5a25380 Compare December 22, 2021 22:39
@ljharb ljharb merged commit 5a25380 into jsx-eslint:master Dec 22, 2021
@duncanbeevers duncanbeevers deleted the util-is-react-import-shadowed branch December 23, 2021 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants