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 rule: prefer-exact-props #1547

Open
wants to merge 11 commits into
base: master
from

Conversation

5 participants
@jomasti
Copy link
Contributor

jomasti commented Nov 19, 2017

This rule checks if non-exact objects are used for defining prop types when using Flow along with using normal objects for prop types instead of something like prop-types-exact to enforce only passing the exact props to a component.

The exact prop wrapper functions for non-Flow usage are configurable by adding the exact property to an object setting in the propWrapperFunctions config option.

Closes #1455

@ljharb
Copy link
Collaborator

ljharb left a comment

Also note the propWrapperFunctions settings, with can be used to specify exact or forbidPropTypes.

Perhaps this warrants a new setting for "exact propType wrappers", that could also be checked everywhere the existing propWrappers are?

Show resolved Hide resolved docs/rules/prefer-exact-props.md
@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Nov 19, 2017

Do you think it should be a global setting instead of just an option for the rule? I'm unsure of the other use cases for it being a global setting.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Nov 19, 2017

@jomasti if it were a global setting, then other rules could access it to "unwrap" propTypes for their own uses.

@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Nov 19, 2017

@ljharb Wouldn't these particular "exact props" functions still be in propWrapperFunctions in order to facilitate that?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Nov 19, 2017

@jomasti right; the idea is that i'd be able to move forbidExtraProps etc out of propWrapperFunctions and into this new setting.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Nov 19, 2017

I guess an alternative schema for the existing setting could be:

"propWrapperFunctions": [
  "Object",
  { "property": "freeze", "object": "Object" },
  { "property": "forbidExtraProps", "exact": true }
]
@maxammann

This comment has been minimized.

Copy link

maxammann commented Sep 11, 2018

What exactly is missing before this can be merged? Can we maybe split this task and add support for libs like airbnb/prop-types in an other task?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Sep 11, 2018

@maxammann that support already exists; see the "propWrapperFunctions" setting in the readme.

This PR needs to be updated per the above comments.

ljharb added a commit to jomasti/eslint-plugin-react that referenced this pull request Dec 3, 2018

[New] Change allowed propWrapperFunctions setting values
This change is adapting the `propWrapperFunctions` setting to allow for
using objects along side the already present strings. This will help
facilitate adding extra attributes to these objects like for exact prop
functions.

Precursor to yannickcr#1547.

@jomasti jomasti force-pushed the jomasti:issue-1455 branch from e5ee336 to 4989a30 Dec 4, 2018

@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Dec 4, 2018

After doing some digging, it looks like the failures on ESLint 3 are due to versions 7.22+ of babel-eslint using eslint-scope over escope if available. This combination of the two results is weird errors for TypeAlias and maybe others. I'm not sure what I should do to rectify this.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 4, 2018

We could modify the dep range, and for eslint 3 in Travis.yml manually downgrade it?

@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Dec 4, 2018

We could modify the dep range, and for eslint 3 in Travis.yml manually downgrade it?

Would you prefer for the before_script to be extracted to a separate script with this extra logic?

@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Dec 4, 2018

Downgrading to a working version of babel-eslint breaks other tests, so it doesn't seem viable.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 4, 2018

Breaks other tests on eslint 3?

@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Dec 4, 2018

Yes, using babel-eslint@7.2.1 with eslint@3 breaks tests that use object spread in type aliases and the short syntax for fragments.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 4, 2018

So what is it exactly about this pr that causes this breakage?

@jomasti

This comment has been minimized.

Copy link
Contributor

jomasti commented Dec 4, 2018

It's looking for the TypeAlias variable in the scope. This works fine on eslint 4 and above because both it and babel-eslint use eslint-scope.

@jomasti jomasti closed this Jan 4, 2019

@jomasti jomasti force-pushed the jomasti:issue-1455 branch from 4989a30 to d2aa260 Jan 4, 2019

@jomasti jomasti changed the title New rule: prefer-exact-props New rule: prefer-exact-prop Jan 4, 2019

@jomasti jomasti changed the title New rule: prefer-exact-prop New rule: prefer-exact-props Jan 4, 2019

@jomasti jomasti reopened this Jan 4, 2019

@jomasti jomasti force-pushed the jomasti:issue-1455 branch 2 times, most recently from ecbb05d to cb6aba0 Jan 4, 2019

Show resolved Hide resolved lib/util/propWrapper.js Outdated
Show resolved Hide resolved lib/util/propWrapper.js Outdated

@ljharb ljharb added the new rule label Jan 4, 2019

jomasti and others added some commits Nov 18, 2017

Add some more test cases and refactor
This adds a couple more test cases that were missed. This also refactors
the rule a bit to clean it up. There is also a workaround for keeping
track of TypeAlias nodes seen to work around an issue with eslint@3 and
babel-eslint preventing the TypeAlias variables being found in the
scope.
Apply suggestions from code review
Co-Authored-By: jomasti <facedelajunk@gmail.com>

@jomasti jomasti force-pushed the jomasti:issue-1455 branch from c072439 to 3a44049 Jan 5, 2019

@ljharb

ljharb approved these changes Jan 8, 2019

Copy link
Collaborator

ljharb left a comment

Seems great!

(I'm going to hold off merging it for awhile while the 7.12 release settles down)

@ljharb ljharb requested review from EvHaus and yannickcr Jan 8, 2019

@EvHaus

This comment has been minimized.

Copy link
Collaborator

EvHaus commented Jan 8, 2019

This rule seems to be eerily similar to no-unused-prop-types. Can you help me understand what the major difference is? We might even want to add a note to the README to explain why you might use this over no-unused-prop-types.

I get that this specifically forces exact definitions in Flow and PropType wrappers, but the underlying reason for that is to avoid unused props in a component -- is that right?

@alexzherdev

This comment has been minimized.

Copy link
Contributor

alexzherdev commented Jan 8, 2019

I could be wrong, but my understanding of this is forbidExtraProps warns at runtime when a component's consumer passes in more than the component needs; whereas no-unused-prop-types warns if the component itself is not using some of the propTypes it declares. So it's two separate things.

@EvHaus

EvHaus approved these changes Jan 8, 2019

Copy link
Collaborator

EvHaus left a comment

That makes sense. Thanks @alexzherdev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment