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

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

Closed
sebmarkbage opened this issue Aug 11, 2015 · 16 comments
Closed

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

sebmarkbage opened this issue Aug 11, 2015 · 16 comments

Comments

@sebmarkbage
Copy link
Collaborator

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 Warn If a Component Doesn't Inherit React.Component Warn If a Component Doesn't Extend React.Component Aug 11, 2015
@rads
Copy link

rads commented Aug 11, 2015

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

@Schniz
Copy link

Schniz commented Aug 11, 2015

@rads 👍🏻

@sebmarkbage
Copy link
Collaborator Author

@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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link

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

@gaearon
Copy link
Collaborator

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
Copy link

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

@NekR
Copy link

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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@sophiebits
Copy link
Collaborator

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

@sebmarkbage
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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.

@sebmck
Copy link
Contributor

sebmck 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 😛

@wentout
Copy link

wentout commented Sep 28, 2020

babel/babel#12115

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