Skip to content

Move valid test case to invalid for no-typos due to ESLint upgrade #1437

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Move valid test case to invalid for no-typos due to ESLint upgrade #1437

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions tests/lib/rules/no-typos.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,6 @@ ruleTester.run('no-typos', rule, {
'}'
].join('\n'),
parserOptions: parserOptions
}, {
// PropTypes declared on a component that is detected through JSDoc comments and is
// declared AFTER the PropTypes assignment does not work.
code: `
MyComponent.PROPTYPES = {}
/** @extends React.Component */
class MyComponent extends BaseComponent {}
`,
parserOptions: parserOptions
}, {
// https://github.com/yannickcr/eslint-plugin-react/issues/1353
code: `
Expand Down Expand Up @@ -763,5 +754,17 @@ ruleTester.run('no-typos', rule, {
}, {
message: 'Typo in declared prop type: objectof'
}]
}, {
// PropTypes declared on a component that is detected through JSDoc comments and is
// declared AFTER the PropTypes only works when ESLint version is >= 4.7.0
Copy link
Member

Choose a reason for hiding this comment

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

We should support both eslint < v4.7, and >= v4.7, and this code should be made to fail as expected in both ranges.

Copy link
Contributor Author

@jseminck jseminck Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm - not sure how we can make it fail with ESLint >= 4.7 😄

It does feel a bit strange to really force a supported case to be unsupported though? Currently we do support both version ranges, but the plugin behaves differently in both... but yes, I agree that's kind of confusing as well.

I'll see what can be done. I think the code to handle it would be like this:

  • If component has JSDoc
  • If static properties are declared before the component is declared
    -> Then ignore propTypes typo checking

Copy link
Member

Choose a reason for hiding this comment

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

To clarify; I think that the code warning or not warning should behave identically in 4.6 and 4.7 - I don't really care which we pick, as long as it doesn't change.

If that's not possible, then that will mean 4.7 was a breaking change, and eslint core needs to revert something so that it again works as expected.

Copy link
Contributor Author

@jseminck jseminck Sep 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We definitely cannot easily detect this case with ESLint 4.6. That's why we had to choose to ignore it until the ESLint issue fixed.

Indeed, 4.7 did some kind of internal refactor of ESLint core. If you're interested, this is the change: eslint/eslint#9283

This is a reference implementation of #9122. With this change, rules don't have to worry about whether node.parent has been set at any given point, because all of the node.parent properties will be assigned by the time the rule gets run.

Copy link
Member

Choose a reason for hiding this comment

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

in that case, we need to explicitly ignore it in eslint ^4.7.

code: `
MyComponent.PROPTYPES = {}
/** @extends React.Component */
class MyComponent extends BaseComponent {}
`,
parserOptions: parserOptions,
errors: [{
message: ERROR_MESSAGE
}]
}]
});