webcomponents / react-integration

Converts web components into React components so that you can use them as first class citizens in your React components.
MIT License
306 stars 31 forks source link

No setter on propTypes for resulting React component #47

Closed dmaj7no5th closed 8 years ago

dmaj7no5th commented 8 years ago

We're defining a static get method for returning propTypes on the component created by this function, but no setter. The result is that if you try to define propTypes you get a runtime error:

const component = reactify(MyWebComponent);

component.propTypes = {
  someAttr: React.PropTypes.string.isRequired
};

// => RUNTIME ERROR. Tried to set value on a property that has no defined setter.

We should consider removing the propTypes if they're offering no value - or maybe create a setter function that just does an Object.assign under the hood

treshugart commented 8 years ago

If you do an Object.assign() in the setter and someone assigns a new object to the value, wouldn't they get unexpected behaviour in that they set the value, but it was merged with the existing values underneath the hood? How would they remove certain values if need be?

We could expose the value as a configurable / writable prop using value. That way they could overwrite as well as Object.assign() themselves if they want. If that's too obtuse, then I agree that the current values may not be more valuable than having a simpler API.

dmaj7no5th commented 8 years ago

That would work as well. Making it configurable / writable just opens it up so that the default propTypes you're defining here could be overwritten and lost. If you want to keep them intact, you'd need to merge them together. I guess the question is whether they are providing any value as-is. If not, then the simplest fix is to remove them altogether.

dmaj7no5th commented 8 years ago

Changed PR to just remove the default propTypes that were being set. They weren't really adding any value, and it breaks the React API. This way there's no confusion on what's being set.