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] destructuring-assignment: remove useContext destructuring check #3583

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

102
Copy link
Contributor

@102 102 commented Jun 1, 2023

It seems that the use of modern context is forced to use the destructuring assignment after #2797; the problem was raised in #3536.

This PR introduces additional flag that won't affect the existing behavior when it is unset, but will allow users to upgrade the lib version without having this rule disabled.

The approach was reviewed and now it consists of the following:

  1. [Fix] destructuring-assignment: Handle destructuring of useContext in SFC #2797 changes were reverted
  2. Additional test cases were added in order to make sure that the value returned from useContext could now be used both with destructuring and without it

Fixes #3536

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #3583 (a93cae7) into master (693860f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head a93cae7 differs from pull request most recent head 7f655f8. Consider uploading reports for the commit 7f655f8 to get more accurate results

@@            Coverage Diff             @@
##           master    #3583      +/-   ##
==========================================
- Coverage   97.62%   97.61%   -0.01%     
==========================================
  Files         132      132              
  Lines        9295     9280      -15     
  Branches     3400     3391       -9     
==========================================
- Hits         9074     9059      -15     
  Misses        221      221              
Impacted Files Coverage Δ
lib/rules/destructuring-assignment.js 97.39% <100.00%> (-0.31%) ⬇️

@ljharb
Copy link
Member

ljharb commented Jun 1, 2023

I'm not sure we need an option - since the return value of useContext isn't always an object, the rule should simply never force destructuring of it.

@102
Copy link
Contributor Author

102 commented Jun 1, 2023

I'm not sure we need an option - since the return value of useContext isn't always an object, the rule should simply never force destructuring of it.

This will basically reduce the changes to reverting #2797, if I understand it correctly (and I’m absolutely ok with this option, can update the PR shortly)

@ljharb
Copy link
Member

ljharb commented Jun 1, 2023

ah, hmm. cc @Zinyon - what are your thoughts here?

@102
Copy link
Contributor Author

102 commented Jun 1, 2023

ah, hmm. cc @Zinyon - what are your thoughts here?

We can also make the option to do the opposite — ignore by default, but allow users to enable the useContext check using the flag

@102
Copy link
Contributor Author

102 commented Jun 7, 2023

Hi @ljharb, @Zinyon, do you have any thoughts on the further steps?

@ed-jinyoung-park
Copy link
Contributor

@102 @ljharb Hi!

seems proper options to handle issues like #3536.
how about making option to ignore by default ?
there can be many cases that does not need destructuring like modern context as single value (not object) i think.

@ljharb
Copy link
Member

ljharb commented Jun 8, 2023

@Zinyon it seems like that since useContext can be any kind of value, that it's simply incorrect to force destructuring of it.

@ed-jinyoung-park
Copy link
Contributor

ed-jinyoung-park commented Jun 8, 2023

@ljharb i agree on second thought. seems better to remove logic for reporting about destruturing useContext that i worked at #2797
https://github.com/jsx-eslint/eslint-plugin-react/pull/3583/files#diff-6351eca58740d2b829e9e8bf54fde9e65b07419cc99a2e96e2b4fdf960969d1aR242-R243

…Context in SFC"

 - [Tests] `destructuring-assignment`: Add more modern context cases

This reverts commit 523db20 / jsx-eslint#2797

Fixes jsx-eslint#3536.
@102 102 changed the title [New] destructuring-assignment: add ignoreUseContext option [Fix] destructuring-assignment: remove useContext destructuring checks Jun 8, 2023
@102 102 changed the title [Fix] destructuring-assignment: remove useContext destructuring checks [Fix] destructuring-assignment: remove useContext destructuring check Jun 8, 2023
@102
Copy link
Contributor Author

102 commented Jun 8, 2023

@ljharb, @Zinyon, the useContext checks were removed (and also a few additional test cases were added to make sure that useContext destructuring is not being enforced)

@ed-jinyoung-park
Copy link
Contributor

@102 thanks for your work !

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

[Bug]: destructuring-assignment enforces destructuring of modern context
4 participants