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

[no-constructed-context-values] Update isJsxContext check to allow for JSXIdentifier ending with Provider #3283

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liuyenwei
Copy link

jsx/no-constructed-context-values currently only works for context providers that are used as <Context.Provider value={..}>

We have a lot of use cases where, instead of exporting the context directly, we explicitly export the provider as a named export. The naming convention for this is typically ${NameOfContext}Provider

The proposed change here is to update the isJsxContext check to take the above scenario into account and run the lint rule for these elements as well.

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #3283 (03645af) into master (c8833f3) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3283      +/-   ##
==========================================
+ Coverage   97.72%   97.73%   +0.01%     
==========================================
  Files         123      123              
  Lines        8745     8746       +1     
  Branches     3170     3173       +3     
==========================================
+ Hits         8546     8548       +2     
+ Misses        199      198       -1     
Impacted Files Coverage Δ
lib/rules/jsx-no-constructed-context-values.js 94.17% <100.00%> (+1.03%) ⬆️

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 c8833f3...03645af. Read the comment docs.

const isJsxContext = (openingElementName.type === 'JSXMemberExpression'
&& openingElementName.property.name === 'Provider')
|| (openingElementName.type === 'JSXIdentifier'
&& openingElementName.name.endsWith('Provider'));
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a convention we can reasonably hardcode into this plugin, I'm afraid.

Copy link
Author

Choose a reason for hiding this comment

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

another option here could be to add a config option for specifying a pattern to test an identifier against.
maybe something like:

jsxIdentifierPattern: RegExp

what do you think about something like ^? happy to update the PR if you're okay with this approach

Copy link
Member

Choose a reason for hiding this comment

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

Allowing regexes in eslint config is a very bad idea, and is a magnet for ReDOS CVEs.

I don't think it's possible to have your convention work with static analysis, I'm afraid. You could perhaps use a pragma comment like we have to identify React components that extend "not React.Component", but you'd still have to put it in every consuming file.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the reply.

re: the regexp, my thought here was that it would be isolated to just the jsx identifier so the risk would be low. also, afaik there are other rules (in this project) that do support regex configs so i thought it would be an acceptable approach (ex: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/lib/rules/boolean-prop-naming.js#L45)

it seems that this should be something that we can detect statically since we know the identifier, it's just a matter of validating that the identifier is a valid context provider. another idea here would be use a function as a config option (i.e. isJsxIdentifierContextProvider) but that feels pretty overkill imo

not trying to be pushy here - just trying to brainstorm ideas to support our use case. would love to avoid forking this rule since 99% of the logic would be the same.

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 definitely open to ideas - it's just got to be something maintainable.

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