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

Extending PropTypes with Ember specific types #6

Closed
Gowiem opened this issue May 25, 2017 · 3 comments
Closed

Extending PropTypes with Ember specific types #6

Gowiem opened this issue May 25, 2017 · 3 comments

Comments

@Gowiem
Copy link
Contributor

Gowiem commented May 25, 2017

Hey @evrowe,

I'm just starting to use the library and I'm really digging it. I like having that explicit contract for a component right in the code.

One thing I have run into is that the prop types that React uses don't have any idea of Ember constructs. For example, Ember uses an Ember.Object to represent a PromiseArray. So when using PropTypes.array for a collection of DS.Model returned from a route it will fail.

I would like to build this into this library and I am hoping to discuss adding Ember specific extensions to the PropTypes object. These could live on PropTypes.ember.* so that the component is explicitly stating 'This is an Ember related prop'.

For example, some proposed additions I think would be useful:

  1. PropTypes.ember.array: Checks if the given component prop is an Ember.Array via Ember.isArray
  2. PropTypes.ember.model($MODEL_CLASS): Checks if the given component prop is an instance of the given DS.Model class.

Is this of interest, I'd be happy to put together a PR that takes a crack at the above!

@DHedgecock
Copy link
Member

Sounds like a good idea, as long as we don't create props validators that aren't value added.

For example, is there value added to creating a PropTypes.PromiseArray vs checking PropTypes.instanceOf(DS.PromiseArray)? I'm assuming instance of works correctly here, let us know if it doesn't)

I also think we should avoid namespacing as much as possible and prefer specific names like PropTypes.dsModel vs PropTypes.ember.model.

@evrowe thoughts?

@evrowe
Copy link
Member

evrowe commented May 25, 2017

@Gowiem Hey Matt, thanks for your feedback and we're stoked you're liking the addon!

I like where this discussion has gone so far. I think there's definitely value in making sure there is support for Ember constructs within this library, since React isn't going to have validations available for Ember-specific classes.

I generally also agree with @DHedgecock that we don't necessarily need to namespace new proptypes so much as name them adequately; this actually simplifies the implementation somewhat.

My suspicion is that PropTypes.instanceOf(DS.PromiseArray) should hypothetically work as-is, but it may require a teensy bit of extra trickery because Ember is Ember.

If it did turn out that the instanceOf approach wasn't sufficient, and that we needed to build some Ember-specific validators, I think we would want to do so along these lines:

PropTypes.dsModel,
PropTypes.emberArray,
PropTypes.promiseArray,
PropTypes.emberComponent
...

I will say that I think the point of PropTypes.instanceOf is to allow for validation against any arbitrary class/type you want without having to write validators for each individual item, and for the time being I tend to prefer that approach if it works as expected. If not, then we should definitely start working on providing proper support.

@evrowe
Copy link
Member

evrowe commented Jun 2, 2017

With the merge of [#8] and the fresh release of 1.4.0, this issue should now be fully addressed.

@evrowe evrowe closed this as completed Jun 2, 2017
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

3 participants