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

Custom propTypes should be able to use mixins. #433

Closed
wants to merge 1 commit into from

Conversation

tommyh
Copy link

@tommyh tommyh commented Oct 16, 2013

Pull request for this issue: #431

Contributing:

  • grunt test and grunt lint both pass
  • I have filled out the CLA under tommyh

@tommyh
Copy link
Author

tommyh commented Oct 17, 2013

@SanderSpies - based on the needs of my application and the example I gave for this pull request (#431), I completely agree with you and @spicyj that DEFINE_MANY_MERGED is the correct solution.

There is one minor point I'd like to bring up, which may/maynot make this pull request still valuable.

There are really 2 features to discuss:

  1. Mixins defining propTypes which can be merged in to a class
  2. Mixins defining functions which can be used by a class' custom propType functions

In my app, I started with option 2, but it turned out that I really needed option 1. Once I figured this out, we had this conversation on irc:

[20:23] irc tommyh: hi, does anyone know why propTypes from mixins get ignored (if the class also has propTypes), instead of reverse merged in? is this by design? or just a nice-to-have feature?
[20:33] irc tommyh: ok, i'm reading more of the source, so I'd like to rephrase my question - does anyone know the reason ReactCompositeComponentInterface.propTypes = SpecPolicy.DEFINE_ONCE. I get why render/shouldComponentUpdate/getInitialState are DEFINE_ONCE, but I don't understand why propTypes/getDefaultProps are. (not saying there isn't a reason, just that it's not instantly obvious to me)
[21:34] irc balpert: tommyh: we could probably change propTypes to DEFINE_MANY_MERGED
[21:34] irc balpert: submit a pull request! ;)
[21:39] irc tommyh: +balpert - very cool. i'll see what i can do

So I will try to start working on option 1 with DEFINE_MANY_MERGED as the solution. But if someone needs option 2, then this pull request is the correct solution.

So the question is - is supporting option 2 worth the performance impact of using call? Speaking objectively, I can't answer that question without more data. How much does this pull request affect performance? Are there any existing perf tests which already show this? As react becomes more stable and widely used, will developers expect the functions in their mixins to be available in propType functions?

I do agree that option 1 is the 90% solution which works for most cases.

@tommyh
Copy link
Author

tommyh commented Oct 18, 2013

If .call doesn't have the negative performance hit, then does this seem like something that should be merged in?

Note: I still plan on working on DEFINE_MANY_MERGED for propTypes - hopefully in the next week or so.

@vjeux
Copy link
Contributor

vjeux commented Dec 26, 2013

#487 is now merged and this is no longer needed :) Thanks @tommyh for the work on this!

@vjeux vjeux closed this Dec 26, 2013
@zpao
Copy link
Member

zpao commented Dec 26, 2013

This was different than #487. This allowed functions in mixins to be called as part of custom proptype

@sophiebits
Copy link
Collaborator

(My #849 includes this change.)

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

Successfully merging this pull request may close these issues.

5 participants