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

[new] Add jsx-props-no-spreading rule #2191

Merged
merged 1 commit into from Mar 12, 2019

Conversation

ashbhir
Copy link
Contributor

@ashbhir ashbhir commented Mar 8, 2019

This adds a rule to disallow JSX props spreading as it reduces both the readability and maintainability of the code. Fixes #1094.

So, instead of passing props like this:

function MyComponent(props) {
  return <MySubComponent {...props} />
}

This rule will enforce passing props like this:

function MyComponent(props) {
  return <MySubComponent one_prop={props.one_prop} two_prop={props.two_prop} />
}

@ljharb
Copy link
Member

ljharb commented Mar 8, 2019

also hopefully like this:

function MyComponent({ one_prop, two_prop }) {
  return <MySubComponent one_prop={one_prop} two_prop={two_prop} />
}

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.

I think this will need an exceptions array option as well, since spreading props is actually necessary for HOCs.

Additionally, I think it would be ideal to provide another option that controls whether this rule applies to custom component elements, or HTML elements, or both.

docs/rules/jsx-props-no-spreading.md Outdated Show resolved Hide resolved
docs/rules/jsx-props-no-spreading.md Outdated Show resolved Hide resolved
lib/rules/jsx-props-no-spreading.js Outdated Show resolved Hide resolved
lib/rules/jsx-props-no-spreading.js Outdated Show resolved Hide resolved
@ljharb ljharb added the new rule label Mar 8, 2019
@ashbhir
Copy link
Contributor Author

ashbhir commented Mar 9, 2019

I think this will need an exceptions array option as well, since spreading props is actually necessary for HOCs.

Additionally, I think it would be ideal to provide another option that controls whether this rule applies to custom component elements, or HTML elements, or both.

Added option allowedFor for catering exceptions, namely: HTMLTags, Components and selective custom components.

@ljharb
Copy link
Member

ljharb commented Mar 9, 2019

hmm. Thanks for adding that! We probably should have discussed the schema a bit more :-)

Let's do something like this:

{
  html: 'enforce'/'ignore',
  custom: 'enforce'/'ignore',
  exceptions: ['ComponentName', 'OtherComponentName'],
}

and make sure it errors if html and custom are both false. An "exception" will always flip the resulting html or custom setting for that component - ie, html set to ignore, with an exception of div will enforce on a div; custom set to enforce with an exception of Foo will ignore Foo.

@ashbhir
Copy link
Contributor Author

ashbhir commented Mar 10, 2019

hmm. Thanks for adding that! We probably should have discussed the schema a bit more :-)

Let's do something like this:

{
  html: 'enforce'/'ignore',
  custom: 'enforce'/'ignore',
  exceptions: ['ComponentName', 'OtherComponentName'],
}

and make sure it errors if html and custom are both false. An "exception" will always flip the resulting html or custom setting for that component - ie, html set to ignore, with an exception of div will enforce on a div; custom set to enforce with an exception of Foo will ignore Foo.

Sure, made the respective changes.
and make sure it errors if htmlandcustom are both false. How do we do this in schema ?

@ljharb
Copy link
Member

ljharb commented Mar 10, 2019

That's a good question :-) it'll probably have to be something like:

allOf: [
  {
    oneOf: [
      bothEnforce,
      htmlEnforceCustomIgnore,
      htmlIgnoreCustomEnforce
    ],
  },
  exceptionsSchema
]

@ashbhir
Copy link
Contributor Author

ashbhir commented Mar 10, 2019

That's a good question :-) it'll probably have to be something like:

allOf: [
  {
    oneOf: [
      bothEnforce,
      htmlEnforceCustomIgnore,
      htmlIgnoreCustomEnforce
    ],
  },
  exceptionsSchema
]

I think the allOf won't work as then one has to mention all properties. But one may just want to mention the exceptions assuming others to be set as default.
Also, do we really need to restrict both ignore mode? What if one wants to ignore everything barring some very important Custom Components. For them the configuration will be:

{
    html: 'ignore',
    custom: 'ignore',
    exceptions: ['CustomComponent', 'img']
}

@ljharb what do you think?

@ljharb
Copy link
Member

ljharb commented Mar 12, 2019

ah true, that's a good point.

I'd like the schema to be as explicit as possible. If both are "ignore", then exceptions must not be empty.

@ashbhir
Copy link
Contributor Author

ashbhir commented Mar 12, 2019

ah true, that's a good point.

I'd like the schema to be as explicit as possible. If both are "ignore", then exceptions must not be empty.

@ljharb made the requested 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.

Looks pretty good, 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.

None yet

2 participants