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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] jsx-no-leaked-render: invalid fixes in coerce mode #3511

Merged

Conversation

akulsr0
Copy link
Contributor

@akulsr0 akulsr0 commented Dec 24, 2022

Fixes #3431

@codecov
Copy link

codecov bot commented Dec 24, 2022

Codecov Report

Merging #3511 (438472f) into master (12fe944) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 438472f differs from pull request most recent head f350303. Consider uploading reports for the commit f350303 to get more accurate results

@@           Coverage Diff           @@
##           master    #3511   +/-   ##
=======================================
  Coverage   97.57%   97.58%           
=======================================
  Files         130      130           
  Lines        9213     9221    +8     
  Branches     3342     3348    +6     
=======================================
+ Hits         8990     8998    +8     
  Misses        223      223           
Impacted Files Coverage Δ
lib/rules/jsx-no-leaked-render.js 98.70% <100.00%> (+0.15%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ljharb ljharb force-pushed the akul/jsx-no-leaked-render-false-fixing branch from 0f4f518 to 2f682b3 Compare December 24, 2022 23:00
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@ljharb ljharb force-pushed the akul/jsx-no-leaked-render-false-fixing branch from 2f682b3 to 0dac159 Compare December 24, 2022 23:05
@ljharb
Copy link
Member

ljharb commented Dec 24, 2022

@akulsr0 seems like tests are failing

@akulsr0
Copy link
Contributor Author

akulsr0 commented Dec 26, 2022

@ljharb I am not sure why tests are failing here, I have tried with different node versions in local, its passing all test cases.

@ljharb
Copy link
Member

ljharb commented Dec 26, 2022

The failures are in eslint < 5.

@ljharb
Copy link
Member

ljharb commented Dec 26, 2022

I’m not sure why the output is different in eslint 3 and 4, but we may have to make the test output condition on eslint version.

@akulsr0
Copy link
Contributor Author

akulsr0 commented Dec 26, 2022

Do we have any example case where output is based on eslint version?

@ljharb
Copy link
Member

ljharb commented Dec 26, 2022

@akulsr0 mostly examples of test cases that are entirely skipped, but

semver.satisfies(eslintPkg.version, '> 3') ? {
code: `
var First = createReactClass({
propTypes: {
/* z */
z: PropTypes.string,
/* a */
a: PropTypes.any
},
render: function() {
return <div />;
}
});
`,
output: `
var First = createReactClass({
propTypes: {
/* a */
a: PropTypes.any,
/* z */
z: PropTypes.string
},
render: function() {
return <div />;
}
});
`,
errors: [
{
messageId: 'propsNotSorted',
line: 7,
column: 13,
type: 'Property',
},
],
} : [],
should hopefully be enough for you to figure out how to adapt it?

@ljharb ljharb force-pushed the akul/jsx-no-leaked-render-false-fixing branch from 438472f to f350303 Compare December 27, 2022 05:43
@ljharb
Copy link
Member

ljharb commented Dec 27, 2022

I tweaked your changes so the test cases still run in eslint 3, and it exposed a bug, which i then fixed. This should hopefully be good to go once tests pass.

@ljharb ljharb merged commit f350303 into jsx-eslint:master Dec 27, 2022
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.

jsx-no-leaked-render performs redundant props check and invalid fix in coerce mode
2 participants