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

Warn If a Component Doesn't Extend React.Component #4599

Closed
sebmarkbage opened this Issue Aug 11, 2015 · 15 comments

Comments

Projects
None yet
@sebmarkbage
Member

sebmarkbage commented Aug 11, 2015

To support arrow functions and plain functions as "components" we need to know if we can call new on them. Examples that won't work:

function PlainFunctionComponent(props) {
  return null;
}

var ArrowFunctionComponent = (props) => <div />;

class ClassComponent {
  render() {
    return <div><PlainFunctionComponent /><ArrowFunctionComponent /></div>;
  }
}

We need to detect if a component is a "function" or a "class" before calling new on it.

We can call new on everything if they're plain functions as long as they return a ReactElement. However, that won't work for null/false/string return values and we want to support those too. We also can't call new on arrow functions. Likewise, we can't NOT call new on classes.

Unfortunately ECMAScript doesn't have a way to detect if something is newable or not.

The current plan is to add our own static flag to React.Component so that we can detect if something is a class.

Any class pattern that doesn't require new should still be possible:

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

I'm not sure how we can distinguish this pattern from one that requires new for warnings though. :(

@sebmarkbage sebmarkbage changed the title from Warn If a Component Doesn't Inherit React.Component to Warn If a Component Doesn't Extend React.Component Aug 11, 2015

@rads

This comment has been minimized.

rads commented Aug 11, 2015

What about checking if ClassComponent.prototype.render is a function?

@Schniz

This comment has been minimized.

Schniz commented Aug 11, 2015

@rads 👍🏻

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Aug 11, 2015

@rads That might be enough. A bit fragile. It also means that classes in ECMAScript can never have a default render function. :)

We might have more than one way of rendering in the future. E.g. prerender / postrender / renderAtIndex. That might still work though.

Will need to check if there's any difference in terms of engine optimizations between these options.

@bloodyowl

This comment has been minimized.

Contributor

bloodyowl commented Aug 11, 2015

I don't think duck-typing is the way to go, much too fragile. having to extend ReactComponent is a bit more annoying for stateless components, but it would provide a lot more possibilities in the future I guess.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Aug 11, 2015

Another solution would be to only allow arrow functions as plain functions since they have undefined prototypes they're easily detectable. E.g. function Foo(props) {} would be new:ed but arrow functions would be call:ed. Kind of sucks for transpilers since they don't follow this part of the spec.

@Lenne231

This comment has been minimized.

Lenne231 commented Aug 11, 2015

@sebmarkbage Is there a performant way for transpilers to change this behavior?

@gaearon

This comment has been minimized.

Member

gaearon commented Aug 11, 2015

having to extend ReactComponent is a bit more annoying for stateless components

But if component is stateless you can just turn it into a function?

@briandipalma

This comment has been minimized.

briandipalma commented Aug 11, 2015

Asking users to brand classes with a Symbol would probably be too awkward?

@NekR

This comment has been minimized.

NekR commented Aug 11, 2015

Asking users to brand classes with a Symbol would probably be too awkward?

With decorators seems more better here. But of course it's too early for decorators.

@threepointone

This comment has been minimized.

Contributor

threepointone commented Aug 11, 2015

However, that won't work for null/false/string return values and we want to support those too.

excited to see this happen. will this also mean not needing to wrap JSX 'interpolated' strings with s? eg <div>time : {Date.now()}</div> currently wraps the latter part with a span tag.

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Aug 11, 2015

@threepointone No, that's still the case. This is unrelated.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Aug 19, 2015

@sebmarkbage Want to do the prototype check or the static flag?

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Aug 19, 2015

typeof Class.prototype.render === 'function' wouldn't work on property initializers with an auto-bound render or some decorator that auto-binds everything.

Extending React.Component seems unnecessarily strict though. :/ Don't know...

@sebmarkbage

This comment has been minimized.

Member

sebmarkbage commented Sep 1, 2015

@sebmck Any plans on giving arrow functions and concise methods .prototype = void 0 in babel?

Basically, we're working around the fact that we can't distinguish them from a plain function which is ambivalent with regard if it should be constructable or not.

I would go as far as saying that function expression/declarations should be considered deprecated as of ES6, and best practice should be to use any of the other forms of creation a function.

@kittens

This comment has been minimized.

Member

kittens commented Sep 1, 2015

@sebmarkbage

Any plans on giving arrow functions and concise methods .prototype = void 0 in babel?

Not really. Although recently I've been leaning more towards correctness over performance, eg. Babel 6.0.0 is going to have TDZ by default. There'd be a bunch of cases we could statically analyse and omit the prototype assignment such as IIFE. Might be worth adding.

I would go as far as saying that function expression/declarations should be considered deprecated as of ES6, and best practice should be to use any of the other forms of creation a function.

Probably wouldn't go that far, I can already feel the pitchforks being raised 😛

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