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

Pure Functions as Stateless Components #3995

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@vslinko
Copy link

vslinko commented Jun 1, 2015

ref #3220

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 1, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 1, 2015

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

};
};

wrappedStatelessFunctions.push({fn: fn, Klass: Klass});

This comment has been minimized.

@syranide

syranide Jun 1, 2015

Contributor

It's safer and faster to store the result as a property on the function (and use WeakMap when it becomes widely supported). It's not super sanitary, but pretty common JS-practice.

This comment has been minimized.

@vslinko
};
};

fn.ReactComponent.displayName = fn.name;

This comment has been minimized.

@vslinko

vslinko Jun 1, 2015

Use function name as component name.

@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 1, 2015

Should I provide context as second argument?

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jun 1, 2015

We should ideally try to avoid caching things on these function objects if possible.

One neat feature of these functions is that they don't need a backing instance which could help performance. We also know that they won't have a shouldComponentUpdate so we could skip the memoization part.

What if instead of wrapping, you just kept executing it until you get a result that can be instantiated?

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jun 1, 2015

One plausible incremental route could be to fork ReactCompositeComponent and see how far we can optimize that branch.

@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 1, 2015

@sebmarkbage I'll try

@sebmarkbage sebmarkbage referenced this pull request Jun 1, 2015

Closed

Umbrella 0.14 #3220

20 of 25 tasks complete
@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 1, 2015

@sebmarkbage @jimfb I'm copied ReactCompositeComponent and remove some waste code. Please review and comment.

@bsansouci

This comment has been minimized.

Copy link

bsansouci commented Jun 1, 2015

Nice work! It won't warn you for not having a render method inside a component though. It'll try to use it as a render function instead of as a constructor. (With babel, you'll get a warning that the constructor can't be used as a function). Maybe you could do something like:

function isStatelessComponentType(type) {
  return (
    typeof type === 'function' &&
    (typeof type.prototype === 'undefined' ||
     type.prototype instanceof ReactComponent) // change this line
  );
}
@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 1, 2015

@bsansouci Sorry, but I don't get it.
Stateless component isn't an subclass of ReactComponent. It's just pure simple function:

function Component(props) {
  return <div>{props.a}</div>;
}

Constructor will be rendered as usual using ReactCompositeComponent.

@bsansouci

This comment has been minimized.

Copy link

bsansouci commented Jun 1, 2015

Oh whoops, I meant NOT an instance of ReactComponent.

@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 1, 2015

Problem is in any classes should be rendered using ReactCompositeComponent not only childs of ReactComponent

@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 1, 2015

So any function that has render in prototype is composite component.

@bsansouci

This comment has been minimized.

Copy link

bsansouci commented Jun 2, 2015

Ok so any component without a render method will make React throw an error then?

@vslinko

This comment has been minimized.

Copy link

vslinko commented Jun 2, 2015

Any function without render in prototype will make React throw error if that function returns not React Element.

@@ -83,6 +106,8 @@ function instantiateReactComponent(node, parentCompositeType) {
// represenations. I.e. ART. Once those are updated to use the string
// representation, we can drop this code path.
instance = new element.type(element);
} else if (isStatelessComponentType(element.type)) {

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

This won't work since we allow module pattern components that may not have a visible prototype. E.g:

function Foo() {
  return {
    render() {
      return <div />;
    }
  };
}

I'm surprised this wasn't covered by a unit test.

This comment has been minimized.

@vslinko

vslinko Jun 12, 2015

Are you sure React should support this kind of definition? What is benefit of this over stateless functions and classes?

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 12, 2015

Member

It was explicitly added in 0.13 because we don't want to be opinionated about how to structure classes. That is especially important for integration with compile-to-js languages.

This comment has been minimized.

@vslinko

vslinko Jun 12, 2015

So, I don't see any option how to check function is stateless or module before ReactCompositeComponentMixin.mountComponent. That means we should merge ReactCompositeComponentMixin and ReactStatelessComponentMixin and turn on optimisations if var inst = new Component(publicProps, publicContext); returns ReactElement.

May be you have other better and simpler ideas?

This comment has been minimized.

@vslinko

vslinko Jun 12, 2015

@sebmarkbage I found a problem with that kind of components while trying to fix this: #4109

This comment has been minimized.

@vslinko

vslinko Jun 12, 2015

@sebmarkbage I'm done with hack that supports stateless and module pattern components at same time.

Now component constructor called two times. I can fix this if you agreed to remove context argument from module pattern components constructor:

// Now
type StatelessComponent = (props) -> ReactElement
type ModulePatternComponent = (props, context) -> ReactComponent

// Should be
type StatelessComponent = (props) -> ReactElement
type ModulePatternComponent = (props) -> ReactComponent
var renderedComponent;
var previousContext = ReactContext.current;
ReactContext.current = this._currentElement._context;
ReactCurrentOwner.current = this;

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

Context here is supposed to be removed. Owner is only used for refs and it is not possible for one of these to have refs so this can be removed.

This comment has been minimized.

@sophiebits

sophiebits Jun 2, 2015

Contributor

Can we keep it in __DEV__ for debugging purposes?

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

hm... Maybe. How about we add it back later? I think it might require a different code path since this should avoid as many internal instances as possible in prod.

This comment has been minimized.

@vslinko
attachRef: function(ref, component) {
var inst = this.getPublicInstance();
var refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
refs[ref] = component.getPublicInstance();

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

There is no public instance associated with this so therefore there is no way to attach a ref this way. This code path can be removed and associated paths too.

This comment has been minimized.

@vslinko
* @internal
*/
getPublicInstance: function() {
return this._instance;

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

This is always null.

This comment has been minimized.

@vslinko

vslinko Jun 12, 2015

React.findDOMNode returns null if this method return null


var shouldUpdate = true;

if (shouldUpdate) {

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

Drop the block, this is constant.

This comment has been minimized.

@vslinko
*
* @private
*/
var nextMountID = 1;

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

This is not needed.

This comment has been minimized.

@vslinko
*/
mountComponent: function(rootID, transaction, context) {
this._context = context;
this._mountOrder = nextMountID++;

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

This is only used for sorting updates, and since this can't get any updates, it is not needed.

This comment has been minimized.

@vslinko
* @internal
*/
mountComponent: function(rootID, transaction, context) {
this._context = context;

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

The context is always available to you as an argument. No need to store it on this instance.

This comment has been minimized.

@vslinko
this._context = nextContext;
inst.props = nextProps;

this._updateRenderedComponent(transaction);

This comment has been minimized.

@sebmarkbage

sebmarkbage Jun 2, 2015

Member

This can be merged in. Lets you get access to context directly.

This comment has been minimized.

@vslinko
@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Jul 21, 2015

@jimfb

I think the key difference is that we don't have to instantiate a component instance and don't have to preserve state, which allows for a whole bunch of performance+memory optimizations.

My static object/class above does just that, technically you could just skip the instantiation and pass the object/class as the instance (EDIT: ok that's not entirely true). It would even appear almost like a regular instance if someone where to try and get a reference to it, not sure why you would though. With the possibility of using option 4 for wrapping a regular function to produce such a static class/object, optionally with any other behaviors included (say shouldComponentUpdate). To put it bluntly, pure function + attributes sounds like React.createClass again whereas what I'm proposing stays entirely unopinionated.

Most importantly, it allows you to use a lambda function as a React component, which is a syntax that allows a whole bunch of patterns that were previously clunky to become practical.

I'm curious what those are and how that actually works out given the "expensive default behavior", it sounds like having to set a magic property on the lambda would defeat the purpose. So you would end passing it through a function anyway at which point you could just go option 4 to begin with I would assume? Then there's also the issue of function() {} !== function() {} totally wreaking havoc on optimizations.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Jul 21, 2015

@syranide Yeah, I'm not saying your alternate syntaxes aren't valid, was just explaining the benefits. The function syntax is fairly natural, and using attributes to specify special behaviors is also fairly natural/flexible. If you prefer the fourth suggestion, you can always implement it yourself to append the attribute. The function syntax doesn't hurt you if you prefer to use a wrapper function instead of setting an attribute.

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Jul 22, 2015

The function syntax doesn't hurt you if you prefer to use a wrapper function instead of setting an attribute.

@jimfb Lambdas with special attributes feels like revisiting React.createClass where React again invents its own opinionated pre-defined behaviors. By instead requiring transformation of lambdas to static classes we remain entirely unopinionated and users can provide whatever transforms they want, no restrictions.

function makePureLambda(render) {
  return {
    shoudComponentUpdate: predefined,
    render: render
  };
}

var MyLambdaComponent = makePureLambda(function() {
  return <div />
});

That and all things considered, including inline lambdas being a really bad idea (as it breaks shouldComponentUpdate). I'm doubting the usefulness of these if their only benefit is simply being less verbose, but comes with significant trade-offs (especially for those less familiar)...? It looks like a feature with scary side-effects being promoted for its terse syntax (which many will use simply for being terse).

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Aug 4, 2015

Lambdas with special attributes feels like revisiting React.createClass where React again invents its own opinionated pre-defined behaviors.

+1

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Aug 4, 2015

Lambdas with attributes are conceptually no different from ES6 classes with attributes. Both are standard javascript constructs with attributes that are understandable by React.

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Aug 4, 2015

@jimfb If the attributes are intended to be used with classes then it kind of makes sense, is that the idea? It sounded like the attributes where for lambdas only.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Aug 4, 2015

@syranide propTypes and defaultProps are currently used on classes when defining your ES6 components.

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Aug 4, 2015

@jimfb "Our" objection is towards introducing opinionated attributes to specifically work around the limitations of lambdas. Classes having (static) properties isn't really controversial.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Aug 4, 2015

@syranide Maybe I'm miss-understanding something, but what's the difference?

class MyComponent
{
  ...
}
MyComponent.defaultProps = {};
module.exports = MyComponent;
function MyComponent()
{
  ...
}
MyComponent.defaultProps = {};
module.exports = MyComponent;

They look the same to me. Define something on your standard-javascript construct, which React understands, for more advanced functionality.

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Aug 4, 2015

@jimfb Ah, we're talking about different things :)

It would be possible to add an attribute later that specifies the stateless function is pure, thus enabling you to make it fast. We've already discussed using attributes to indicate special properties on stateless functions, so this isn't an unreasonable solution.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Aug 4, 2015

Right... but it's the same...

function MyComponent()
{
  ...
}
MyComponent.pureRender = true;
module.exports = MyComponent;

One could easily imagine the pureRender flag which does intelligent optimizations if you specify that your parameters conform to tripple-equals equality. The details of which options are allowed (and what the defaults are) are still being determined, but the use of attributes doesn't seem like something that would/should be objectionable, since the exact same style is used for classes without objection.

@syranide

This comment has been minimized.

Copy link
Contributor

syranide commented Aug 4, 2015

@jimfb #3995 (comment) ... so I think we agree, if this is a feature you intend for classes too, then I see nothing wrong with it (whether it is an attribute or w/e). But when you proposed it, it sounded like a lambda-only feature (to make up for their simplicity) and that didn't sound right.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Aug 4, 2015

Ok, cool, glad we agree :). Thanks for the good discussion!

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Aug 4, 2015

@jimfb Yeah, I think you're right now. It trips me up a bit that you could wrap a function in another function calling it, and some behaviors no longer apply, but OTOH it's fair: we don't know if the new function also exhibits them.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Aug 4, 2015

@gaearon It's worth noting that the wrapper function can return an element instead of manually invoking the delegate, thereby preserving the original behaviors because the nested component renders independently. The wrapper can also copy the values from the delegate, thereby exposing those behaviors while avoiding a second component render. Or the wrapper can expose different behaviors if so desired. This behavior is the same as HOCs using class based components, and is fully flexible without making assumptions about the propagation of behaviors. I'd argue that these are the ideal semantics.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Aug 11, 2015

@jimfb Agreed. 👍

@sophiebits sophiebits removed this from the 0.14 milestone Aug 24, 2015

@probablyup probablyup referenced this pull request Sep 8, 2015

Merged

0.14 RC blog post #4797

@sophiebits

This comment has been minimized.

Copy link
Contributor

sophiebits commented Sep 8, 2015

For anyone following along on this issue, an alternate variant of this was merged in #4587 but we might take this implementation or a similar one in a future release.

@chicoxyzzy

This comment has been minimized.

Copy link
Contributor

chicoxyzzy commented Oct 14, 2015

any plans to merge this into 0.15?

@vslinko

This comment has been minimized.

Copy link

vslinko commented Nov 22, 2015

@sebmarkbage @spicyj @syranide @jimfb
Sorry for mentioning, what we will do with my PR?
I could start it again with from the fresh master branch.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Nov 22, 2015

We would need to refactor out the part of the initialization that makes it necessary to create a "ReactStatelessOrCompositeComponentWrapper" and avoid the allocation there. If you can avoid the extra allocation we can take it. See ReactReconciler for the direction we're going. No internal classes, just function calls.

@jimfb jimfb referenced this pull request Jan 6, 2016

Closed

pureRender static property #5788

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Feb 3, 2016

We took #4587. At the very least, this would require a ton of work. Merge conflicts, six months old, this PR is dead.

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