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

Feature request - PropType.*.name #8310

Closed
tuchk4 opened this issue Nov 16, 2016 · 12 comments
Closed

Feature request - PropType.*.name #8310

tuchk4 opened this issue Nov 16, 2016 · 12 comments

Comments

@tuchk4
Copy link

tuchk4 commented Nov 16, 2016

Feature request: Fill name attribute or add static toString() method for all PropTypes.

For example:

const StringPropType = PropTypes.string;
expect(StringPropType.name).toEqual('checkType: String');

const ArrayOfBoolType = PropTypes.arrayOf(PropTypes.bool);
expect(ArrayOfBoolType.name).toEqual('bound checkType: Array of Bool');

const ShapeType = PropTypes.shape({
  foo: PropTypes.bool,
  bar: PropTypes.number
});
expect(ShapeType.name).toEqual('bound checkType: Shape of foo:Bool, bar:Number'

It maybe useful when debugging.
In my case - I need to equal components prop types and throw exception if there are same props. So if generate names for prop types - they could be compared.

Current behaviour

Only primitive types could be compared (string, bool, number).
Complex types (shape, arrayOf) - each time returns new bound function. So it could not be compared by value.

Problems with custom prop types

Will generate "undefined" name for custom prop like this:

customProp: function(props, propName, componentName) {
    if (!/matchme/.test(props[propName])) {
      return new Error(
        'Invalid prop `' + propName + '` supplied to' +
        ' `' + componentName + '`. Validation failed.'
      );
    }
  },

but it fixed by adding function name:

customProp: function customProp(props, propName, componentName) {

btw. Same feature implemented at tcomb lib

@tuchk4
Copy link
Author

tuchk4 commented Nov 16, 2016

If it is OK I can make PR

@tuchk4 tuchk4 changed the title Feature request - fill PropType.*.name attribtue Feature request - PropType.*.name Nov 16, 2016
@gaearon
Copy link
Collaborator

gaearon commented Nov 16, 2016

Can you please explain why you're testing presence of PropTypes? How is this helping you? Some examples from product code would be helpful.

@tuchk4
Copy link
Author

tuchk4 commented Dec 13, 2016

Hi @gaearon. Sorry for late response.

I will provide few examples when this is feature is useful

React storybook and react storybook info addon

Info addon parses all components from story and show table with component prop types and default props.
And it work correctly with all simple types (number, string, node) and with isRequired flag.

Link to PropTypesMap generation https://github.com/storybooks/react-storybook-addon-info/blob/89bc19038c09b0b3439f5abb8de91315228d47e5/src/components/PropTable.js#L4

So if implement requested feature - this addon will work more correctly and for all prop types.

Forgekit

Forgekit merge prop types and default props from component and components features.
But sometimes there are collision between defined prop type names - when same prop types defined in few features. But if prop type is also the same - probably it is not a collision.

I mean:

  • feature1:
foo: PropTypes.string
  • feature2:
foo: PropTypes.string

This case is OK, but:

  • feature1:
foo: propTypes.string
  • feature2:
foo: propTypes.bool

This case is a prop type collision.

But now this prop types matching is not reachable.

@tuchk4
Copy link
Author

tuchk4 commented Dec 13, 2016

btw

maybe save autogenerated name attribute (which is a String) is not very good idea for complex props (nested shapes with arrayOfs etc). But is you are OK with feature request - we can discuss it.

@tuchk4
Copy link
Author

tuchk4 commented Dec 22, 2016

@gaearon any updates?)

@gaearon
Copy link
Collaborator

gaearon commented Dec 22, 2016

I'm worried that something like 'bound checkType: Shape of foo:Bool, bar:Number' won't scale nicely in large codebases that have recursive / deeply nested existing types. I'd be happy to take a look at a PR with tests to get a better idea of what you want to support, but not sure this is something we would want.

@tuchk4
Copy link
Author

tuchk4 commented Jan 4, 2017

Provide simple PR with feature implementation.
Instead of using toString() or name attr with definition as string - I have added struct attribute and describe prop structure as object.

I hope it will be completely clear if look at tests
Custom props behaviour is also presented at tests.

If it is OK - I will complete PR (clean code, remove duplications etc).

@vladfr
Copy link

vladfr commented Jan 17, 2017

+1
Another good use case is tests, you may want to do different things depending on proptypes.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2017

But sometimes there are collision between defined prop type names - when same prop types defined in few features. But if prop type is also the same - probably it is not a collision.

This actually sounds like something that will break in future versions of React. You should not be using PropTypes for any behavior that is observable in production. For example, we plan to replace the PropTypes checkers with a single throwing type checker in prod (to reduce React byte size). In this case PropTypes.string will be the same as PropTypes.number in production. So if you relied on that for the logic, your app could break in prod.

This is why I’m sceptical about this direction. I don’t want to make PropTypes more powerful because then people will start relying on them to implement “magic” behaviors that we don’t support and could break in the future.

@tuchk4
Copy link
Author

tuchk4 commented Jan 17, 2017

But sometimes there are collision between defined prop type names - when same prop types defined in few features. But if prop type is also the same - probably it is not a collision.

@gaearon I agree, this direction is probably wrong.

But another cases are still useful and only for dev env

Another case from project I have worked with:

There a lot of components with complex props.
Values for props should be editable at control panel (for different locals or even according to current user)
Props values are passed to components via redux connect.

static propTypes = {
  strings: PropTypes.shape({
    title: PropTypes.string,
    subtitle: PropTypes.string,
    text: PropTypes.string,
  }).
  images: PropTypes.shape({
    image: PropTypes.object,
  }),
  options: PropTypes.shape({
    backgroundColor: PropTypes.string,
  }),
  links: PropTypes.shape({
    appstore: PropTypes.string,
    playmarket: PropTypes.string,
  }),
}

So to build form at control panel to edit component values developers should read component source as text and then parse prop types. Also it can be implemented via:

  • babylon to get ast tree and parse as object.
  • react-docgen api to get component doc object and get props. But as I know there were some problems with complex (shape, enum) props.

PropTypes struct maybe is most suitable solution.

And I think there are lot of other examples where this feature would be useful.


Anyway this is not really important feature among other React pull requests or other React needs :)

@oleblaesing
Copy link

I want to create a small faker module for React that generates random data based on the components propTypes. This feature would be very helpful, because there is currently no "native" solution to handle this problem in a comfortable way. You can look at my current idea of implementation here: oleblaesing/react-proptypes-faker

@aweary
Copy link
Contributor

aweary commented Apr 9, 2017

PropTypes have been removed from React core and now exist as a separate package prop-types. Any future feature requests, bug reports, or changes should be directed to the new repo at https://github.com/reactjs/prop-types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants