Stateless Components

[iamdustan]

Starts support for Stateless Component detection and documentation. Currently this adds an isStatelessComponent util method. I’ll start working on working this in to the resolvers now, but wanted early review on this to see if I missed anything.

[fkling]

Looks good so far. One remark is actually covered by a typo in this test :D (note the missing e in React.creatElement.

Seems like it accepts any function which calls another function as react component.

[iamdustan]

ha, well, the issue is that I accidentally deleted the check to ensure that I was calling createElement in the visitor below so basically everything is matched 😇

I’ve gotten a bit stuck on trying to duplicate isReactCreateClassCall to isReactCreateElementCall which handles ensuring the correct createElement method is called and it looks up to the root import React bit to ensure it’s the real react.

in src/utils/resolveToValue.js path.scope.lookup(node.name) is always returning undefined.

[fkling]

In your test? You'd also have to add var React = require('react'); to the test code.

[iamdustan]

Yeah, I’ve done that to each...well, technically I’ve done import React from 'react'; or import R from 'react';.

I’m bouncing for dinner, but should have the next push sometime tomorrow with the next set of questions :)

[fkling]

👍 Really appreciate your effort :)

[iamdustan]

Oh! Found my sticking point. Since I’m calling recast.visit(node, {}) in the lcontainsJSXElementOrReactCreateElementCall function I’m losing all of recasts internal state

[iamdustan]

Managed to get through this. Passing the root path instead of path.node didn’t lose state.

[iamdustan]

I notice you have class components normalize to static propTypes rather than <ES6-style properties. I suspect I’ll need to the propTypeHandler to also read from <ES6-style properties for stateless components unless you have any other ideas.

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

Component.propTypes = {};

[fkling]

I attribute it to bad design decisions at the beginning ;) Everything kind of expects that the result of the resolver is a single AST node and extracts data from there.

I guess we could introduce some new interface, ResolverResult, and have implementations for class, object literals (React.createClass) and stateless functions, to abstract from the different representations. Could be quite some work though.

Handlers shouldn't necessarily what kind of definition they are dealing with. That custom logic could be implemented in the getMemberValuePath method though, which already takes care of the differences between class declarations and object literals (React.class).

[iamdustan]

I’m currently working on extending the getMemberValuePath to also getMemberExpressionValuePath and see how far that takes me. I think it’ll only need to handle variable declarations, function expressions, and function declarations. This should avoid needing an new interface and will continue building on the early design decisions :)

All of my API transformation projects have terrible design decisions. And be “terrible” I mean “no”.

[fkling]

All of my API transformation projects have terrible design decisions. And be “terrible” I mean “no”.

:)

[iamdustan]

Well....I think this is actually working now.. 😮

[iamdustan]

hmmm....not quite working, but close.

[iamdustan]

Alright, fixed my final commit. This is now working for stateless function components in my current codebase as expected :)

[iamdustan]

@fkling I probably have until EOD to finish this up then will have to bow out for a while. I want to prioritize supported use cases versus not supported use cases. This is the known issues ordered by what I think the priority should be. What do you think?

• isStatelessComponent is too loose in saying true. Should scope this better (high priority)

const NotAComponent = () => {
  const Component = () => React.createElement('div', null);
  const AlsoAComponent = () => React.createElement('div', null);

  return {Component, AlsoAComponent};
}

• hash of components (low/no priority)

const bag = {
  Component() { return React.createElement('div', null); },
  AlsoAComponent() { return React.createElement('div', null); }
};

bag.Components.propTypes = {
  /**
   * Hey I'm a string
   */
  foo: PropTypes.string.isRequired
};

[iamdustan]

Also e4c867c is of note for the integration-style test proving that is indeed working correctly. :)

[fkling]

I don't think we need this, do we? What case does this capture which isn't captured otherwise?

[iamdustan]

I added this as part of supporting some of the permutations also being added to gaearon/babel-plugin-react-transform; namely this format: https://github.com/gaearon/babel-plugin-react-transform/pull/34/files#diff-9d6f6ff3fa6b5b63c09190bd7a2efb95R15

Honestly I’m not sure if that is captured or handled correctly yet. 😇

[fkling]

I think this should be handled by the other visitors since methods are just represented as function expressions in the AST.

On Oct 7, 2015, at 12:51 PM, Dustan Kasten <[email protected]mailto:[email protected]> wrote:

In src/resolver/findAllComponentDefinitions.jshttps://github.com//pull/28#discussion_r41437429:

recast.visit(ast, {

• visitFunctionDeclaration: statelessVisitor,
• visitFunctionExpression: statelessVisitor,
• visitProperty: statelessVisitor,

I added this as part of supporting some of the permutations also being added to gaearon/babel-plugin-react-transform; namely this format: https://github.com/gaearon/babel-plugin-react-transform/pull/34/files#diff-9d6f6ff3fa6b5b63c09190bd7a2efb95R15

Honestly I’m not sure if that is captured or handled correctly yet. [:innocent:]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/28/files#r41437429.

[fkling]

This is pretty awesome work, thank you so much! Regarding priorities:

isStatelessComponent is too loose in saying true. Should scope this better (high priority)

Definitely that one. Not sure how tricky this one is though.

hash of components (low/no priority)

I wonder what the use cases are for doing something like this. This would be covered by finding all component definitions anyway though, wouldn't it?

---

I was wondering about this following case:

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

Object.assign(Foo, {
  displayName: 'Foo',
  propTypes: { ... }
});

Do you think that would be a common use case? Or are propTypes and displayName the only properties one would add to stateless components? I guess defaultProps too. Maybe this is a "nice to have".

[iamdustan]

isStatelessComponent is too loose in saying true. Should scope this better (high priority)

I have a couple ideas to try. The first is just to filter on all ReturnStatement’s in the body of a possible stateless component and detect if it’s a ReactElement. Though I’m not certain if these will be considered valid components off-hand. +@spicyj if I need to account for those. I don’t think they are at current..

const IsThisAComponent => null;
const IsThisAComponent => 'string';
const IsThisAComponent => 7;
const IsThisAComponent => [<div key="first" />, <div key="last" />];

The weakness is this will definitely fail for the following permutations:

const Component(props) {
  if (Math.random() > 0.5) return <div />;
  else return <span />;
}

const Component(props) {
  return (Math.random() > 0.5) ? <div /> : <span />;
}

Alternatively I could do some scope crawling and ensure that JSXElement and React.createElement are in not in a nested scope. This would handle the following case:

var HOC = (options) => {
  const Component = (props) => <div />
  Component.propTypes = {};
  return {Component};
}

Sadly, I think that to be totally accurate we’ll need to track all possible return values and ensure each is a JSXElement or React.createElement. E.g. lookup return values for both truthyFunc and falsyFunc in the following example:

const truthyFunc = () => <div />;
const falsyFunc = () => <span />;
const Component = (props) => props ? truthyFunc() : falsyFunc();

[sophiebits]

@iamdustan Those aren't currently valid components but will be (at least the one with null) in 0.15.

[iamdustan]

thanks @spicyj. I’ll punt that until 0.15 :)

[fkling]

In the worst case we can also introduce a doclet, like @react or @react-component to mark functions as React components that would be hard to identify otherwise.

[iamdustan]

@fkling this now supports a pretty wide array of use cases. It’s not quite as bullet proof as I’d like, but this is all I have within me for now. Let me know what you think and if you want to block on anything.

[iamdustan]

This function for this isn’t quite recursive enough. While all of these resolve as expected, the following two cases do not resolve correctly yet:

const ComponentG = function(props) {
  var nested = {
    helpers: {
      comp() { return <div>{props.children}</div>; }
    }
  };
  return nested.helpers.comp();
};
const ComponentF = function(props) {
  var other = () => <div>{props.children}</div>;
  var helpers = {
    comp: other
  };
  return helpers.comp();
};

[iamdustan]

This works like a champ.

[iamdustan]

@fkling I’m good here unless you come up with additional code examples that fail. A thorough review to undo the cobwebs I’ve created over the past week could likely help clean up the code in here.

[fkling]

@iamdustan: I'll try to get to it tomorrow or Wednesday!

[iamdustan]

Another thought I had while adding so many use cases was: do we even want to support all the possible ways to do stateless components for this? The idea of these stateless function components is for simplicity. Documenting only a subset of these could help encourage folks to keep things simple.

[fkling]

do we even want to support all the possible ways to do stateless components for this?

Yep, good point. My answer to this is: no. We probably already don't support every possible way to create a React component and that's OK IMO.
A side effect of this project is that people have to follow certain guidelines to make it work, which in turn hopefully positively impacts consistency / familiarity across different projects / code bases.

Not sure I can get to it this week (will be away tomorrow for the rest of the week) and there is another issue I'd like to address before releasing a new major release (and I think this should be a major release because it could pickup components that haven't been considered as such).

[iamdustan]

👍 can’t slow me down! https://github.com/iamdustan/react-docgen/tree/stateless-components-dist :)

[fkling]

This is done right? (because we use a different way to resolve this information)

[iamdustan]

Why yes I do believe it is!

[jherr]

👍

[iamdustan]

Friendly ping for review.

[fkling]

Sorry, I thought I would be able to get something else in, but don't really have much time. I'm not so sure anymore this requires a major version bump. In the worst case it just finds more components.

A minor version bumpv should suffice an which case I can get this in now. What do you think?

[iamdustan]

I’ve been using it without issues since the PR was opened so it feels generally safe to me. It shouldn’t cause any regressions on createClass or extends React.Component so it should be fine with just a minor, IMO.

[fkling]

Merged in 9f27006 and released as v2.4.

Thank you so much for this!

[iamdustan]

Woohoo! Made it. Thanks!