Skip to content

Stateless Components #28

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

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions src/utils/__tests__/isStatelessComponent-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* Copyright (c) 2015, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
*/

/*global jest, describe, beforeEach, it, expect*/


jest.autoMockOff();

describe('isStatelessComponent', () => {
var isStatelessComponent;
var expression, statement, parse;

beforeEach(() => {
isStatelessComponent = require('../isStatelessComponent');
({expression, statement, parse} = require('../../../tests/utils'));
});

describe('Stateless Function Components with JSX', () => {
it('accepts simple arrow function components', () => {
var def = parse(
'var Foo = () => <div />'
).get('body', 0).get('declarations', [0]).get('init');
expect(isStatelessComponent(def)).toBe(true);
});

it('accepts simple function expressions components', () => {
var def = parse(
'let Foo = function() { return <div />; };'
).get('body', 0).get('declarations', [0]).get('init');
expect(isStatelessComponent(def)).toBe(true);
});

it('accepts simple function declaration components', () => {
var def = parse('function Foo () { return <div /> }').get('body', 0);
expect(isStatelessComponent(def)).toBe(true);
});
});

describe('Stateless Function Components with React.createElement', () => {
it('accepts simple arrow function components', () => {
var def = parse(
'var Foo = () => React.creatElement("div", null);'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Really appreciate your effort :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

).get('body', 0).get('declarations', [0]).get('init');

expect(isStatelessComponent(def)).toBe(true);
});

it('accepts simple function expressions components', () => {
var def = parse(
'let Foo = function() { return React.createElement("div", null); };'
).get('body', 0).get('declarations', [0]).get('init');

expect(isStatelessComponent(def)).toBe(true);
});

it('accepts simple function declaration components', () => {
var def = parse('function Foo () { return React.createElement("div", null); }').get('body', 0);
expect(isStatelessComponent(def)).toBe(true);
});
});

describe('Stateless Function Components inside module pattern', () => {
it('', () => {
var def = parse(`
var Foo = {
Bar() { return <div />; },
Baz: function() { return React.createElement('div'); },
['hello']: function() { return React.createElement('div'); },
render() { return 7; }
}
`).get('body', 0).get('declarations', 0).get('init');

var bar = def.get('properties', 0);
var baz = def.get('properties', 1);
var hello = def.get('properties', 2);
var render = def.get('properties', 3);

expect(isStatelessComponent(bar)).toBe(true);
expect(isStatelessComponent(baz)).toBe(true);
expect(isStatelessComponent(hello)).toBe(true);
expect(isStatelessComponent(render)).toBe(false);
});
});

describe('is not overzealous', () => {
it('does not accept declarations with a render method', () => {
var def = statement(`
class Foo {
render() {
return React.createElement('div', null);
}
}
`);
expect(isStatelessComponent(def)).toBe(false);
});

it('does not accept React.Component classes', () => {
var def = parse(`
var React = require('react');
class Foo extends React.Component {
render() {
return <div />;
}
}
`).get('body', 1);

expect(isStatelessComponent(def)).toBe(false);
});

it('does not accept React.createClass calls', () => {
var def = statement(`
React.createClass({
render() {
return <div />;
}
});
`);
expect(isStatelessComponent(def)).toBe(false);
});

});
});

76 changes: 76 additions & 0 deletions src/utils/isStatelessComponent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Copyright (c) 2015, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/

import isReactModuleName from './isReactModuleName';
import isReactComponentClass from './isReactComponentClass';
import isReactCreateClassCall from './isReactCreateClassCall';
import match from './match';
import recast from 'recast';
import resolveToModule from './resolveToModule';
import resolveToValue from './resolveToValue';

var {types: {namedTypes: types}} = recast;

const validPossibleStatelessComponentTypes = [
'Property',
'FunctionDeclaration',
'FunctionExpression',
// TODO: maybe include these variants for safety:
// https://github.com/benjamn/ast-types/blob/master/def/es6.js#L31
// https://github.com/estree/estree/issues/2
// 'ArrowExpression', 'ArrowFunction'
'ArrowFunctionExpression'
];

function containsJSXElementOrReactCreateElementCall(node) {
var visited = false;
recast.visit(node, {
visitJSXElement(path) {
visited = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, that would also consider foo in

function foo() {
  return function bar() {
    return <div />;
  }
}

as React component, i.e. any function that contains JSX is considered to be a React component. I wonder if we can narrow this down more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I’ve thought of this as well. After I get through the current cases with detecting propTypes (last step I believe) then I’ll look into maybe doing something like a return value lookup of each function rather than the current heuristic of “contains JSX or React.createElement”. I suspect this could get a bit tricky or annoying or untenable if return value is something imported from another module, though. I intend on not supporting that :)

this.traverse(path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wouldn't need to traverse deeper if we already found our answer. You could just return false; to indicate that the traversal is done for this node.

},

visitCallExpression(path) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, that would consider any function that calls another function as component. Is that intentional?

visited = true;
this.traverse(path);
}
})

return visited;
}

/**
* Returns `true` if the path represents a function which returns a JSXElment
*/
export default function isStatelessComponent(
path: NodePath
): bool {
var node = path.node;

if (validPossibleStatelessComponentTypes.indexOf(node.type) === -1) {
return false;
}

if (node.type === 'Property') {
if (isReactCreateClassCall(path.parent) || isReactComponentClass(path.parent)) {
return false;
}
}

if (containsJSXElementOrReactCreateElementCall(node)) {

return true;
}

return false;
}