0.11.4 false positive about jsinspect HOT 6 CLOSED

I'm pretty sure jsinspect 0.9 and below just completely ignored any JSX and kept parsing the files. The tool was completely rewritten in 0.10 as I found people were mostly interested in copy-paste detection, and the API was pretty inconsistent. As a result, thresholds will likely need to be tweaked.

That said, that's definitely a bug! Looks like there's 2 issues that need to be addressed:

• End line for the sequence of JSX nodes is incorrect
• JSX ASTs tend to be quite verbose, will probably need to make them more compact so as to avoid tripping up at lower thresholds

Thanks for letting me know!

from jsinspect.

End line for the sequence of JSX nodes is incorrect

Fixed in 20a16c9 and published a new patch release. The issue is that NodeUtils.getChildren isn't guaranteeing children order. It worked for most nodes, e.g. function params get visited before its body, but not JSX. Will probably update the function to peak at node location before caching attribute order. Previous versions of jsinspect hardcoded attribute traversal order... but it's not very future-proof.

JSX ASTs tend to be quite verbose, will probably need to make them more compact so as to avoid tripping up at lower thresholds

This is still an issue, but working on a fix!

from jsinspect.

Quick explanation as to why it's not completely a false positive, but a higher threshold might be necessary for JSX files. Let's look at this small snippet from the first example:

start={{x: 0.0, y: 0.0}} end={{x: 0.0, y: 1.0}}
    locations={[0, 1]}

This small of a snippet breaks down into the following 25 nodes

JSXIdentifier (start), JSXExpressionContainer, ObjectExpression, ObjectProperty, Identifier (x), NumericLiteral (0), ObjectProperty, Identifier (y), NumericLiteral (0), JSXAttribute, JSXIdentifier (end), JSXExpressionContainer, ObjectExpression, ObjectProperty, Identifier (x), NumericLiteral (0), ObjectProperty, Identifier (y), NumericLiteral (1), JSXAttribute, JSXIdentifier (locations), JSXExpressionContainer, ArrayExpression, NumericLiteral (0), NumericLiteral 1

Token-based copy paste detection would probably even label that small snippet as having ~30-40 tokens. So this would hint that the LinearGradient element could probably benefit from a helper/wrapper/component where all these properties are already set.

As a temp fix, a threshold of 45 or more might be more appropriate. However, this might improve by correcting the JSX node attribute traversal order.

from jsinspect.

And as a note, here's what would be reported after the latest patch release (note that it's not just a single line)

from jsinspect.

This has been fixed in 119e32d

This makes up 40 nodes:

  <LinearGradient
    start={{x: 0.0, y: 0.0}} end={{x: 0.0, y: 1.0}}
    locations={[0, 1]}
    colors={['#074595', '#6589A4']}
    style={Styles.

With 0.12.2, setting the threshold to 41 will avoid catching this LinearGradient. Thanks!

from jsinspect.

Awesome, thanks for the fast turnaround! :) 🥇

from jsinspect.