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] no-unused-state: avoid a crash #3258

Merged
merged 1 commit into from Apr 3, 2022

Conversation

WillyLiaoWH
Copy link
Contributor

Try to fix #3240 with the smallest change.

Fixes jsx-eslint#3240.

Co-authored-by: Willy Liao <willy.liao@appier.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
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; but first we'd need a test case that fails without the fix.

@WillyLiaoWH
Copy link
Contributor Author

Unfortunately, I cannot reproduce it in the test environments. But the error still occurs in my working environment. Here is the sample code that failed in my working environment. The key point is that if I don't declare it in the arrow function form, it would be fine. It would be great if you could spend a few minutes checking the problem. You can just close the PR if you cannot find the root cause either. Thanks for your work again.

import React, { PureComponent } from 'react';

class TestNoUnusedState {
  constructor(props) {
    super(props);
    this.state = {
      id: null,
    };
  }

  static getDerivedStateFromProps = (props, state) => {
    if (state.id !== props.id) {
      return {
        id: props.id,
      };
    }

    return null;
  };

  render() {
    return <h1>{this.state.id}</h1>;
  }
}

export default TestNoUnusedState;

@ljharb
Copy link
Member

ljharb commented Apr 3, 2022

Your test case reveals another problem, but it passes without your change.

ljharb added a commit to ljharb/eslint-plugin-react that referenced this pull request Apr 3, 2022
@ljharb
Copy link
Member

ljharb commented Apr 3, 2022

oops, it does fail in older node versions :-) i'll update this PR.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #3258 (4876d54) into master (b9b1bee) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3258      +/-   ##
==========================================
+ Coverage   97.68%   97.69%   +0.01%     
==========================================
  Files         121      121              
  Lines        8584     8586       +2     
  Branches     3117     3118       +1     
==========================================
+ Hits         8385     8388       +3     
+ Misses        199      198       -1     
Impacted Files Coverage Δ
lib/rules/no-unused-state.js 99.23% <66.66%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9b1bee...4876d54. Read the comment docs.

@ljharb ljharb merged commit 4876d54 into jsx-eslint:master Apr 3, 2022
@ljharb ljharb changed the title Fix issue #3240 [Fix] no-unused-state: avoid a crash Apr 3, 2022
@WillyLiaoWH
Copy link
Contributor Author

Thanks for the fix 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants