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.

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

[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
Draft

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

Show file tree
Hide file tree
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
13 changes: 8 additions & 5 deletions lib/rules/jsx-no-constructed-context-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,15 @@ module.exports = {
return {
JSXOpeningElement(node) {
const openingElementName = node.name;
if (openingElementName.type !== 'JSXMemberExpression') {
// Has no member
return;
}

const isJsxContext = openingElementName.property.name === 'Provider';
// Consider an element a context provider if the name is either:
// - identifier and ends with Provider (i.e. SomeContextProvider)
// - member expression and has Provider as a property (i.e. SomeContext.Provider)
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.


if (!isJsxContext) {
// Member is not Provider
return;
Expand Down
17 changes: 17 additions & 0 deletions tests/lib/rules/jsx-no-constructed-context-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ ruleTester.run('react-no-constructed-context-values', rule, {
{
code: '<Context.Provider value={props}></Context.Provider>',
},
{
code: '<ContextProvider value={props}></ContextProvider>',
},
{
code: '<Context.Provider value={100}></Context.Provider>',
},
Expand Down Expand Up @@ -155,6 +158,20 @@ ruleTester.run('react-no-constructed-context-values', rule, {
},
}],
},
{
// Invalid because object construction creates a new identity
// Duplicate of above test but using an identifier as the context provider name
code: 'function Component() { const foo = {}; return (<ContextProvider value={foo}></ContextProvider>) }',
errors: [{
messageId: 'withIdentifierMsg',
data: {
variableName: 'foo',
type: 'object',
nodeLine: '1',
usageLine: '1',
},
}],
},
{
// Invalid because array construction creates a new identity
code: 'function Component() { const foo = []; return (<Context.Provider value={foo}></Context.Provider>) }',
Expand Down