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

Added new functionality: React.defineComponentMethod #2031

Closed
wants to merge 2 commits into from

Conversation

antikus
Copy link

@antikus antikus commented Aug 12, 2014

New method React.defineComponentMethod lets us define new methods with a specific policy ("DEFINE_MANY", "DEFINE_MANY_MERGED" and etc.). It provides extra flexibility when you need to have some methods that should collect data and so on.

@zpao
Copy link
Member

zpao commented Aug 12, 2014

There are a few technical qualms I have about this. But first, let's decide if we even want something like this. I do wish you would have proposed this somewhere before just doing it. Discussions should be had before making non-trivial architectural changes.

@sebmarkbage @jordwalke @petehunt thoughts on this in general? Opening up the doors for anybody to define how any method on a component can behave sounds like a recipe for compatibility disaster.

@sebmarkbage
Copy link
Collaborator

Global method overrides creates compatibility issues. We could possibly do this on a component basis, but that defeats the purpose of having mixins do all the boilerplate. I think explicit chaining of super calls would solve this issue with minimal extra boilerplate: #1380

@antikus
Copy link
Author

antikus commented Aug 13, 2014

Thanks @zpao -

I'm absolutely open for discussions. Just didn't know how to start (where to write) and this way is seemed the easiest to start discussion.

@zpao, I think you should be agreed that we definitely want something like this and as @sebmarkbage mentioned there is a discussion about this problematique #1380. Thanks sebmarkbage that directed me to this discussion.

@sebmarkbage concerns:

Global method overrides creates compatibility issues.

In the solution I provided we do not let people override existent methods. But, sure, now I see that there can appear some issues and it may be not the best way to do this.

We could possibly do this on a component basis

Yeah, I thought about this, but when you need the same logic for all components it doesn't seem to be DRY if we use mixins.

@syranide
Copy link
Contributor

Isn't this kind of solved by itself in 0.12? (unless I got it wrong)

Then, you'll supply the class and not necessarily have it created by React.createClass, which means that you can simply augment the prototype or create the class however you like. You can do this already by reaching into MyClass.type.prototype (or augmenting the class object you send it), I haven't tested it and perhaps there's something preventing auto-bind from working there?

EDIT: Hmm, perhaps I misunderstood the actual request here, but it seems like turning React.createClass into a framework for creating classes is to go the wrong way (people want inheritance, etc, etc and drawing a line is rather arbitrary).

@antikus
Copy link
Author

antikus commented Aug 14, 2014

@syranide, I don't offer

turning React.createClass into a framework for creating classes

What I did is let people create extra React component-based methods that will have the same behavior React already provided (DEFINE_MANY, DEFINE_MANY_MERGED and etc) for React standard methods (getInitialState, getChildContext and etc).

@syranide
Copy link
Contributor

@anatolikurtsevich Sorry, I was a bit unclear perhaps, what I meant was that this goes one step in that direction (in my opinion), inheritance is another and there are plenty of other requests for similar features. I don't know how the devs feel, but I feel as if React.createClass is a bit of an abomination, React shouldn't concern itself with how classes are created, it's a very grey and complex area and subject to much debate (especially as ES6 classes will be around some day).

The React mixin-behavior with the magic DEFINE_MANY flags is nice, but it's also very very weird. If this is not something that is a traditional (mixin) pattern, then perhaps the solution is not to have DEFINE_MANY at all, but to e.g. have addLifeCycleListener which mixins would be free to call during initialization. Mixins could be applied as a call from the constructor to MixinX.constructor.call(this) rather than Mixins: [MixinX].

I'm not necessarily proposing that as the solution (I'm sure there are better ones), but as an example, it uses only traditional patterns to solve the problem and would move the entire responsibility of how to create classes away from React, rather than give React more responsibility. It's now a clear-cut division of responsibility and React no longer forces users into a specific "interpretation of classes". React can certainly still provide helpers.

Again, this is up to the devs, but that's how I see it.

@sebmarkbage
Copy link
Collaborator

I'll close this out and refer to #1380 which does provide a solution to this problem but we might need a pre-ES6 alternative to do this as well. Let's handle it all in one issue.

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

Successfully merging this pull request may close these issues.

4 participants