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

Projects
None yet
2 participants
@ashbhir
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

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} />
}
@ljharb
Copy link
Collaborator

ljharb left a comment

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.

Show resolved Hide resolved 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

@ljharb ljharb added the new rule label Mar 8, 2019

@ashbhir

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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.

@ljharb

ljharb approved these changes Mar 12, 2019

Copy link
Collaborator

ljharb left a comment

Looks pretty good, thanks!

@ljharb ljharb force-pushed the ashbhir:jsx-props-no-spreading branch from a835a3f to 752de70 Mar 12, 2019

@ljharb ljharb merged commit 752de70 into yannickcr:master Mar 12, 2019

2 of 5 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 97.523%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.