fix(scope-manager): incorrect reference for this within a jsx identifier #4535
Conversation
|
Thanks for the PR, @juank1809! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
|
❌ Deploy Preview for typescript-eslint failed. 🔨 Explore the source changes: 6b9bbdc 🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/62196b83fb4c76000789d0dd |
There was a problem hiding this comment.
thanks for putting this fix up!
This is almost correct!
If we inspect the output from the TS and babel JSX transforms, we can see that another case we need to handle is when this is by itself:
import React from 'react';
class Hello extends React.Component<{tag: () => JSX.Element}> {
inline() {
return [
<this.props.tag />,
<this />,
];
}
}ts playground
babel playground
So we need to handle the more generic case of when the JSXIdentifier has the .name of "this"
Codecov Report
@@ Coverage Diff @@
## main #4535 +/- ##
==========================================
+ Coverage 86.97% 92.45% +5.47%
==========================================
Files 5 330 +325
Lines 215 10600 +10385
Branches 60 3053 +2993
==========================================
+ Hits 187 9800 +9613
- Misses 22 555 +533
- Partials 6 245 +239
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I should probably add this to the docs. And then you can run the test using This will generate a corresponding |
There was a problem hiding this comment.
snapshot is looking good - no dangling references to this in the analysis which is perfect!
I think you can simplify this a little by moving this logic into the JSXIdentifier selector and have it in one place.
JSXIdentifier is only valid in the following positions:
JSXOpeningElement.name(which is analysed)JSXClosingElement.name(not analysed at all)JSXMemberExpression.property(not analysed at all)JSXMemberExpression.object(analysed only in aJSXOpeningElement.name)JSXNamespacedName.name(not analysed at all)JSXNamespacedName.namespace(analysed only in aJSXOpeningElement.name)
So by my reckoning this should be safe to just do within JSXIdentifier!
Otherwise this LGTM
PR Checklist
thiswithin a JSXIdentifier #4055Overview
Currently when a JSX Identifier has name="this" is treated like a variable. This PR will ignore the JSXIdentifier when its name is "this" and thus not referencing it