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

Jump to bottom

Add support for propTypes wrapped in a function #1253

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Add support for propTypes wrapped in a function #1253

merged 1 commit into from
Jun 16, 2017

Conversation

dustinsoftware
Copy link
Contributor

PropTypes would previously get ignored if wrapped in forbidExtraProps.

ljharb
ljharb requested changes Jun 12, 2017
View reviewed changes
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!

However, this behavior should be behind an option (array of strings), and that option should include an explicit identifier name for that function - ie, this should not be considered unless forbidExtraProps is in the eslint config.

@dustinsoftware
Copy link
Contributor Author

Is this what you had in mind?

ljharb
ljharb requested changes Jun 13, 2017
View reviewed changes
lib/rules/no-unused-prop-types.js Outdated
@@ -44,6 +44,9 @@ module.exports = {
},
skipShapeProps: {
type: 'boolean'
},
extraPropsFunc: {
Copy link
Member

Choose a reason for hiding this comment

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

i think this would be better as an array of unique strings - that way multiples could be provided.

Let's also rename it to "propWrapperFunctions"

@dustinsoftware
Copy link
Contributor Author

Ok, I think it's ready for another pass :)

ljharb
ljharb requested changes Jun 13, 2017
View reviewed changes
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.

LGTM pending an update to the docs :-)

lib/rules/no-unused-prop-types.js Outdated
type: 'string'
propWrapperFunctions: {
type: 'array',
items: {
Copy link
Member

Choose a reason for hiding this comment

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

please add uniqueItems: true so that jsonschema enforces uniqueness

ljharb
ljharb approved these changes Jun 14, 2017
View reviewed changes
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.

LGTM!

It'd be nice to rebase this down to a single commit.

@ljharb ljharb requested review from yannickcr, lencioni and EvHaus June 14, 2017 04:24
@dustinsoftware
Copy link
Contributor Author

👍 rebased. Thanks for the quick feedback.

@ljharb ljharb mentioned this pull request Jun 15, 2017
Closed
lencioni
lencioni approved these changes Jun 15, 2017
View reviewed changes
lib/rules/no-unused-prop-types.js Outdated
case 'CallExpression':
if (
configuration.propWrapperFunctions &&
configuration.propWrapperFunctions.indexOf(propTypes.callee.name) !== -1 &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be worth converting the propWrapperFunctions to a structure with constant time lookup, like a Set, to keep this path fast.

ljharb
ljharb requested changes Jun 15, 2017
View reviewed changes
lib/rules/no-unused-prop-types.js Outdated
case 'CallExpression':
if (
configuration.propWrapperFunctions &&
configuration.propWrapperFunctions.indexOf(propTypes.callee.name) !== -1 &&
Copy link
Member

Choose a reason for hiding this comment

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

that's a really great point. this should definitely be converted to a Set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated

@dustinsoftware dustinsoftware force-pushed the forbidExtraProps branch 2 times, most recently from 1520791 to 5d05311 Compare June 16, 2017 03:05
ljharb
ljharb requested changes Jun 16, 2017
View reviewed changes
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.

Almost there :-)

lib/rules/no-unused-prop-types.js Outdated
@@ -56,6 +63,9 @@ module.exports = {
var configuration = Object.assign({}, defaults, context.options[0] || {});
var skipShapeProps = configuration.skipShapeProps;
var customValidators = configuration.customValidators || [];
var propWrapperFunctions = configuration.propWrapperFunctions ?
new Set(configuration.propWrapperFunctions) : new Set();
Copy link
Member

Choose a reason for hiding this comment

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

var propWrapperFunctions = new Set(configuration.propWrapperFunctions || []);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dargh, thanks...should have caught that one

@dustinsoftware
Add support for propTypes wrapped in a function
5770f29 
PropTypes would previously get ignored if wrapped in forbidExtraProps.
@ljharb ljharb merged commit c568d49 into jsx-eslint:master Jun 16, 2017
@ljharb
Copy link
Member

ljharb commented Jun 16, 2017

@dustinsoftware are there other rules that could benefit from this information?

If so, maybe we should move it to "settings", and read from it in multiple rules?

@flats flats mentioned this pull request Jun 20, 2017
Closed
@mqklin
Copy link

mqklin commented Jun 22, 2017

Has it been published?

@ljharb
Copy link
Member

ljharb commented Jun 22, 2017

Nope, not yet.

EvHaus
EvHaus reviewed Jun 22, 2017
View reviewed changes
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM

@ljharb ljharb mentioned this pull request Aug 10, 2017
Closed
@dependencies dependencies bot mentioned this pull request Aug 25, 2017
Open
@renovate renovate bot mentioned this pull request Sep 9, 2024
Merged
1 task
@renovate renovate bot mentioned this pull request Oct 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EvHaus EvHaus EvHaus left review comments

@lencioni lencioni lencioni approved these changes

@ljharb ljharb ljharb approved these changes

@yannickcr yannickcr Awaiting requested review from yannickcr

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dustinsoftware @ljharb @mqklin @lencioni @EvHaus