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

Disable legacy lifecycle methods linting in scope of no-deprecated rule for now #2069

Merged
merged 1 commit into from
Dec 7, 2018

Conversation

sergei-startsev
Copy link
Contributor

@sergei-startsev sergei-startsev commented Dec 2, 2018

As per Dan's request, legacy lifecycle methods have been "excluded" from no-deprecated rule for now since the methods aren't formally deprecated yet.

UPDATED: The original PR has been broken down into 2 parts, the current includes changes related to no-deprecated rule. The 2nd PR is #2075.

Summary

  • Legacy lifecycle methods linting has been disabled in no-deprecated rule until it's decided which release deprecates them. React version 16.999.0 has been set for componentWillMount, componentWillUpdate, componentWillReceiveProps lifecycle methods to disable lint errors for now.
  • rename-unsafe-lifecycles codemod details have been added to no-deprecated rule.
  • no-unsafe rule has been adjusted to handle all unsafe life-cycle methods including their aliases. Developers who want to avoid using of legacy lifecycle methods can enable it today.
  • Instructions on updating components have been adjusted in no-unsafe rule to be consistent with React runtime warnings.
  • Documentation pages have been updated.
  • Corresponding unit tests have been updated/added.

@gaearon, @ljharb could you take a look?

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.

Just to confirm: this PR will not cause any new errors to be issued, it will only reduce errors for those with a version set low enough?

README.md Outdated
@@ -102,7 +102,7 @@ Enable the rules that you would like to use.
* [react/no-children-prop](docs/rules/no-children-prop.md): Prevent passing children as props
* [react/no-danger](docs/rules/no-danger.md): Prevent usage of dangerous JSX properties
* [react/no-danger-with-children](docs/rules/no-danger-with-children.md): Prevent problem with children and props.dangerouslySetInnerHTML
* [react/no-deprecated](docs/rules/no-deprecated.md): Prevent usage of deprecated methods, including component lifecyle methods
* [react/no-deprecated](docs/rules/no-deprecated.md): Prevent usage of deprecated methods
Copy link
Member

Choose a reason for hiding this comment

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

this description should remain, since it's still valid given the proper version settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

deprecated.componentWillMount = [
'16.3.0',
'16.999.0',
Copy link
Member

Choose a reason for hiding this comment

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

these are a semver-patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has to be semver-MINOR, otherwise it doesn't suppress deprecation warnings for the existing React versions (16.3.0 - 16.6.3) where these component lifecycle methods aren't deprecated yet.

Copy link
Member

Choose a reason for hiding this comment

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

why? a minor is for adding new options - this isn't adding a new feature, it's just removing warnings for 16.3-16.6 users. Can you explain more?

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 might have misunderstood your first comment. What semver did you mean? React or eslint-plugin-react? I agree that current PR can be considered as a patch of eslint-plugin-react.

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 talking about this repo :-)

lib/rules/no-unsafe.js Outdated Show resolved Hide resolved
@sergei-startsev sergei-startsev force-pushed the legacy-lifecycle-methods branch 3 times, most recently from bbe9436 to f5aeced Compare December 3, 2018 23:34
@sergei-startsev
Copy link
Contributor Author

@ljharb PR doesn't cause any new errors for react/recommended preset, it disables linting of legacy lifecycle methods until it's decided which release deprecates them.

To allow developers to prevent using of unsafe methods in ^16.3.0 React versions, no-unsafe rule has been adjusted. The rule isn't in the recommended list.

@ljharb
Copy link
Member

ljharb commented Dec 4, 2018

Not being in the recommended list isn't solely the issue - if no-unsafe is going to be creating new errors for any existing consumer of it, it's potentially a breaking change.

@sergei-startsev
Copy link
Contributor Author

I find it unlikely that no-unsafe rule is used without no-deprecated or code contains both legacy lifecycle methods and their aliases, however it can be theoretically. I could propose to split the PR to two:

  • the 1st includes disabling no-deprecated warnings (no breaking changes);
  • the 2nd PR will contain adjustments for no-unsafe (potential breaking changes)

Thoughts?

@ljharb
Copy link
Member

ljharb commented Dec 4, 2018

Certainly separating it out to get the non-breaking parts in sooner is a great idea. It'd be great, however, if we could find a way to avoid the breaking changes on no-unsafe as well.

- Exclude legacy life-cycle methods from`no-deprecated` rule until it's decided which release deprecates them.
- Add details regarding `rename-unsafe-lifecycles` codemod to automatically update components
@sergei-startsev sergei-startsev force-pushed the legacy-lifecycle-methods branch from 376f5c2 to 70d4e71 Compare December 6, 2018 22:33
@sergei-startsev
Copy link
Contributor Author

@ljharb There're no breaking changes in updated PR now.

@ljharb ljharb merged commit 35c0b4d into jsx-eslint:master Dec 7, 2018
@ghost ghost mentioned this pull request Jan 12, 2019
1 task
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.

2 participants