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

[v6] PropTypes not checked in custom elements ? #871

Closed
vladshcherbin opened this issue Apr 27, 2016 · 18 comments
Closed

[v6] PropTypes not checked in custom elements ? #871

vladshcherbin opened this issue Apr 27, 2016 · 18 comments
Milestone

Comments

@vladshcherbin
Copy link
Contributor

Hey, I've got a custom component and want to check the passed props:

<Field name="title" label="Title" component={Input} />

In Input component I want to make sure that label prop is passed, but the propTypes does not work in Input element. Am I doing something wrong?

@erikras
Copy link
Member

erikras commented Apr 27, 2016

propTypes does not work in Input element

I'm not sure what you mean by that. You can set

Input.propTypes = {
  label: PropTypes.string.isRequired
}

and then not pass label to Field (to be passed on to Input) and the propType warning isn't thrown?

Is that what you mean by "does not work"?

@vladshcherbin
Copy link
Contributor Author

@erikras yes, even a simple element test:

import React, { PropTypes } from 'react'

const Input = (props) => (
  <div className="form-group">
    <label htmlFor={props.name}>{props.label}</label>
    <input id={props.name} {...props} />
  </div>
)

Input.propTypes = {
  name: PropTypes.string.isRequired,
  label: PropTypes.string.isRequired,
  type: PropTypes.string.isRequired
}

export default Input

This gives the warning:

<Input />

This does not:

<Field name="title" component={Input} />

@erikras
Copy link
Member

erikras commented Apr 27, 2016

Very interesting. Will investigate. 🔍

@erikras
Copy link
Member

erikras commented Apr 27, 2016

Crazy.

Stateless Functions

const EmailInput = props => <input type="email" {...props}/>
EmailInput.propTypes = { placeholder: React.PropTypes.string.isRequired }

This gives a prop types warning:

<Field name="email" component={props =>
  <EmailInput {...props}/>
}/>

And this does not:

<Field name="email" component={EmailInput}/>

Component Classes

If you do this

class EmailInput extends React.Component {
  render() {
    return <input type="email" {...this.props}/>
  }
}
EmailInput.propTypes = { placeholder: React.PropTypes.string.isRequired }

...it works fine even with the

<Field name="email" component={EmailInput}/>

...syntax.

Conclusion

There must be something that the JSX syntax is doing that is running the stateless function through a prop types check.

If you give Field a component class, redux-form is calling React.createFactory() on it to convert it into a function that can be called with props.

We need a React expert. @gaearon, got a sec?

@gaearon
Copy link
Contributor

gaearon commented Apr 27, 2016

I don't quite understand. Can you show me an example that doesn't validate PropTypes without Redux Form API? As in, just a standalone example demonstrating the problem.

@gaearon
Copy link
Contributor

gaearon commented Apr 27, 2016

The problem is here:

isClass(component) ? React.createFactory(component) : component

You are bypassing React completely by calling functional components directly. This way their props are not checked, they don't appear in DevTools, and React has no knowledge about them.

Generally this shouldn't be necessary. Just always use createFactory. Or just use createElement(component, props) directly because the factory isn't really that useful. Or use JSX which becomes createElement, but in this case remember to make the variable pascal case so JSX picks it up.

@erikras
Copy link
Member

erikras commented Apr 27, 2016

Here's my fiddle that I was making when you looked at the code yourself. :-)

@erikras
Copy link
Member

erikras commented Apr 27, 2016

Changing my fiddle code to

const Field = props => {
  const { component, ...rest } = props
  const Component = component
  return <Component {...rest}/>
}

is no good, because then it fails with the React.DOM.input factory.

const Field = props => {
  const { component, ...rest } = props
  const factory = React.createFactory(component)
  return factory(rest)
}

is also no good for the same reason. Is there an isFactory() predicate I can use?

@gaearon
Copy link
Contributor

gaearon commented Apr 27, 2016

My point is you don’t ever need isClass. React knows how to render stateless components, just let it render them 😉

@erikras
Copy link
Member

erikras commented Apr 27, 2016

Yes, I understand that, but I also want to be able to accept factories, like React.DOM.input, and React doesn't know how to render those as components.

Perhaps I have to provide my own stateless component wrappers for the input factories, e.g.

import { input, textarea, select } from 'redux-form'
...
<Field component={input}/>

...but I have some users who are not using JSX and therefore (I think) would find factory support useful.

@gaearon
Copy link
Contributor

gaearon commented Apr 27, 2016

This is what Field should look like in your fiddle:

const Field = props => {
  const { component, ...rest } = props
  return React.createElement(component, rest);
}

Factories are not component types. They are just helpers for getting an element. React.createElement(React.DOM.input) won’t work because React.DOM.input is not a component—I think this is actually correct because your API is supposed to accept a component.

React.DOM factories will be removed eventually. Please don’t rely on them. This is just an artifact that is only useful for people who prefer JSX.

@gaearon
Copy link
Contributor

gaearon commented Apr 27, 2016

In your case, <Field component='input' /> is all that is necessary for native components. Because a component type can be either a string or a custom component (function or class). These are valid inputs to createElement.

@erikras
Copy link
Member

erikras commented Apr 27, 2016

Excellent!! Cheers.

@erikras
Copy link
Member

erikras commented Apr 28, 2016

Glad you got to look at this @ooflorent. You're the one not using JSX, right? Nothing about not accepting factories bothers you?

@ooflorent
Copy link
Contributor

Well, not really. I'll just wrap Field in my codebase to accept them.
Moreover, I may revise my position since a lot of optimizations target JSX.

@gaearon
Copy link
Contributor

gaearon commented Apr 28, 2016

Since React encourages wrapping with factories at the import site I don't see a problem for factory users either. For native components, they would pass a string. For custom components, they would pass an unwrapped type that they import anyway.

@ooflorent ooflorent modified the milestone: next-6.0.0 Apr 28, 2016
@erikras
Copy link
Member

erikras commented May 4, 2016

Fix published as v6.0.0-alpha-7.

@lock
Copy link

lock bot commented Jun 3, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants