From ff33fc80daffb3d8803eca026b01a1c9ef2079a6 Mon Sep 17 00:00:00 2001 From: Daniel Tschinder Date: Wed, 17 Apr 2019 13:59:40 -0700 Subject: [PATCH] Handle computed properties correctly and do not fail generation (#340) * Handle computed properties correctly and do not fail generation * Resolve identifiers and correctly use string/number literals * Fix lint --- .eslintrc.js | 4 + src/__tests__/__snapshots__/main-test.js.snap | 21 ++ src/__tests__/fixtures/component_20.js | 17 ++ .../componentMethodsHandler-test.js.snap | 25 ++ .../defaultPropsHandler-test.js.snap | 238 ++++++++++++++++++ .../flowTypeHandler-test.js.snap | 50 ++++ .../propTypeHandler-test.js.snap | 89 +++++++ .../__tests__/componentMethodsHandler-test.js | 29 +++ .../__tests__/defaultPropsHandler-test.js | 132 +++++----- .../__tests__/flowTypeHandler-test.js | 14 ++ .../__tests__/propTypeHandler-test.js | 32 ++- src/handlers/componentMethodsHandler.js | 5 +- src/handlers/defaultPropsHandler.js | 9 +- src/handlers/flowTypeHandler.js | 7 +- src/handlers/propTypeHandler.js | 7 +- .../getMethodDocumentation-test.js.snap | 13 + .../__snapshots__/getPropType-test.js.snap | 28 +++ .../__tests__/getMethodDocumentation-test.js | 20 ++ src/utils/__tests__/getPropType-test.js | 22 ++ src/utils/__tests__/getPropertyName-test.js | 95 +++++++ src/utils/getMethodDocumentation.js | 4 +- src/utils/getPropType.js | 4 +- src/utils/getPropertyName.js | 36 ++- src/utils/isReactComponentMethod.js | 2 +- src/utils/setPropDescription.js | 10 +- 25 files changed, 824 insertions(+), 89 deletions(-) create mode 100644 src/__tests__/fixtures/component_20.js create mode 100644 src/handlers/__tests__/__snapshots__/componentMethodsHandler-test.js.snap create mode 100644 src/handlers/__tests__/__snapshots__/defaultPropsHandler-test.js.snap create mode 100644 src/handlers/__tests__/__snapshots__/propTypeHandler-test.js.snap create mode 100644 src/utils/__tests__/__snapshots__/getMethodDocumentation-test.js.snap create mode 100644 src/utils/__tests__/getPropertyName-test.js diff --git a/.eslintrc.js b/.eslintrc.js index ec9a5352bea..2d229af2f02 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -27,5 +27,9 @@ module.exports = { 'no-unused-vars': 'off', }, }, + { + files: 'src/**/__tests__/*-test.js', + env: { jest: true }, + }, ], }; diff --git a/src/__tests__/__snapshots__/main-test.js.snap b/src/__tests__/__snapshots__/main-test.js.snap index 979a62a1b5c..6f7551200d6 100644 --- a/src/__tests__/__snapshots__/main-test.js.snap +++ b/src/__tests__/__snapshots__/main-test.js.snap @@ -1027,3 +1027,24 @@ Object { }, } `; + +exports[`main fixtures processes component "component_20.js" without errors 1`] = ` +Object { + "description": "", + "displayName": "Button", + "methods": Array [], + "props": Object { + "@computed#children": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"default\\"", + }, + "description": "This is a test", + "required": false, + "type": Object { + "name": "string", + }, + }, + }, +} +`; diff --git a/src/__tests__/fixtures/component_20.js b/src/__tests__/fixtures/component_20.js new file mode 100644 index 00000000000..71876904f4b --- /dev/null +++ b/src/__tests__/fixtures/component_20.js @@ -0,0 +1,17 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +const Button = () => ( +
+); + +Button.propTypes = { + /** This is a test */ + [children]: PropTypes.string.isRequired, +}; + +Button.defaultProps = { + [children]: "default", +}; + +export default Button; diff --git a/src/handlers/__tests__/__snapshots__/componentMethodsHandler-test.js.snap b/src/handlers/__tests__/__snapshots__/componentMethodsHandler-test.js.snap new file mode 100644 index 00000000000..f31d43fa66f --- /dev/null +++ b/src/handlers/__tests__/__snapshots__/componentMethodsHandler-test.js.snap @@ -0,0 +1,25 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`componentMethodsHandler should handle and ignore computed methods 1`] = ` +Array [ + Object { + "docblock": "The foo method", + "modifiers": Array [], + "name": "@computed#foo", + "params": Array [ + Object { + "name": "bar", + "optional": undefined, + "type": Object { + "name": "number", + }, + }, + ], + "returns": Object { + "type": Object { + "name": "number", + }, + }, + }, +] +`; diff --git a/src/handlers/__tests__/__snapshots__/defaultPropsHandler-test.js.snap b/src/handlers/__tests__/__snapshots__/defaultPropsHandler-test.js.snap new file mode 100644 index 00000000000..f7c4a11a2a8 --- /dev/null +++ b/src/handlers/__tests__/__snapshots__/defaultPropsHandler-test.js.snap @@ -0,0 +1,238 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`defaultPropsHandler ClassDeclaration with static defaultProps should find prop default values that are imported variables 1`] = ` +Object { + "foo": Object { + "defaultValue": Object { + "computed": true, + "value": "ImportedComponent", + }, + }, +} +`; + +exports[`defaultPropsHandler ClassDeclaration with static defaultProps should find prop default values that are literals 1`] = ` +Object { + "abc": Object { + "defaultValue": Object { + "computed": false, + "value": "{xyz: abc.def, 123: 42}", + }, + }, + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "baz": Object { + "defaultValue": Object { + "computed": false, + "value": "[\\"foo\\", \\"bar\\"]", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler ClassExpression with static defaultProps should find prop default values that are literals 1`] = ` +Object { + "abc": Object { + "defaultValue": Object { + "computed": false, + "value": "{xyz: abc.def, 123: 42}", + }, + }, + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "baz": Object { + "defaultValue": Object { + "computed": false, + "value": "[\\"foo\\", \\"bar\\"]", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler Functional components with default params should find default props that are literals 1`] = ` +Object { + "abc": Object { + "defaultValue": Object { + "computed": false, + "value": "{xyz: abc.def, 123: 42}", + }, + }, + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "baz": Object { + "defaultValue": Object { + "computed": false, + "value": "[\\"foo\\", \\"bar\\"]", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler Functional components with default params should find prop default values that are imported variables 1`] = ` +Object { + "foo": Object { + "defaultValue": Object { + "computed": true, + "value": "ImportedComponent", + }, + }, +} +`; + +exports[`defaultPropsHandler Functional components with default params should override with defaultProps if available 1`] = ` +Object { + "abc": Object { + "defaultValue": Object { + "computed": false, + "value": "{xyz: abc.def, 123: 42}", + }, + }, + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "baz": Object { + "defaultValue": Object { + "computed": false, + "value": "[\\"foo\\", \\"bar\\"]", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler Functional components with default params should work with aliases 1`] = ` +Object { + "abc": Object { + "defaultValue": Object { + "computed": false, + "value": "{xyz: abc.def, 123: 42}", + }, + }, + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "baz": Object { + "defaultValue": Object { + "computed": false, + "value": "[\\"foo\\", \\"bar\\"]", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler Functional components with default params should work with no defaults 1`] = `Object {}`; + +exports[`defaultPropsHandler ObjectExpression handles computed properties 1`] = ` +Object { + "@computed#bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler ObjectExpression ignores complex computed properties 1`] = ` +Object { + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler ObjectExpression should find prop default values that are literals 1`] = ` +Object { + "abc": Object { + "defaultValue": Object { + "computed": false, + "value": "{xyz: abc.def, 123: 42}", + }, + }, + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, + "baz": Object { + "defaultValue": Object { + "computed": false, + "value": "[\\"foo\\", \\"bar\\"]", + }, + }, + "foo": Object { + "defaultValue": Object { + "computed": false, + "value": "\\"bar\\"", + }, + }, +} +`; + +exports[`defaultPropsHandler should only consider Property nodes, not e.g. spread properties 1`] = ` +Object { + "bar": Object { + "defaultValue": Object { + "computed": false, + "value": "42", + }, + }, +} +`; diff --git a/src/handlers/__tests__/__snapshots__/flowTypeHandler-test.js.snap b/src/handlers/__tests__/__snapshots__/flowTypeHandler-test.js.snap index 38a6c0e16a0..6d6c30da71f 100644 --- a/src/handlers/__tests__/__snapshots__/flowTypeHandler-test.js.snap +++ b/src/handlers/__tests__/__snapshots__/flowTypeHandler-test.js.snap @@ -1,5 +1,55 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`flowTypeHandler TypeAlias class definition for flow <0.53 ignores hash map entry 1`] = ` +Object { + "bar": Object { + "description": "", + "flowType": Object {}, + "required": false, + }, +} +`; + +exports[`flowTypeHandler TypeAlias class definition for flow >=0.53 with State ignores hash map entry 1`] = ` +Object { + "bar": Object { + "description": "", + "flowType": Object {}, + "required": false, + }, +} +`; + +exports[`flowTypeHandler TypeAlias class definition for flow >=0.53 without State ignores hash map entry 1`] = ` +Object { + "bar": Object { + "description": "", + "flowType": Object {}, + "required": false, + }, +} +`; + +exports[`flowTypeHandler TypeAlias class definition with inline props ignores hash map entry 1`] = ` +Object { + "bar": Object { + "description": "", + "flowType": Object {}, + "required": false, + }, +} +`; + +exports[`flowTypeHandler TypeAlias stateless component ignores hash map entry 1`] = ` +Object { + "bar": Object { + "description": "", + "flowType": Object {}, + "required": false, + }, +} +`; + exports[`flowTypeHandler does support utility types inline 1`] = ` Object { "foo": Object { diff --git a/src/handlers/__tests__/__snapshots__/propTypeHandler-test.js.snap b/src/handlers/__tests__/__snapshots__/propTypeHandler-test.js.snap new file mode 100644 index 00000000000..59d2b63e348 --- /dev/null +++ b/src/handlers/__tests__/__snapshots__/propTypeHandler-test.js.snap @@ -0,0 +1,89 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`propTypeHandler React.createClass handles computed properties 1`] = ` +Object { + "@computed#foo": Object { + "required": true, + "type": Object {}, + }, + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler React.createClass ignores complex computed properties 1`] = ` +Object { + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler class definition class property handles computed properties 1`] = ` +Object { + "@computed#foo": Object { + "required": true, + "type": Object {}, + }, + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler class definition class property ignores complex computed properties 1`] = ` +Object { + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler class definition static getter handles computed properties 1`] = ` +Object { + "@computed#foo": Object { + "required": true, + "type": Object {}, + }, + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler class definition static getter ignores complex computed properties 1`] = ` +Object { + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler stateless component handles computed properties 1`] = ` +Object { + "@computed#foo": Object { + "required": true, + "type": Object {}, + }, + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; + +exports[`propTypeHandler stateless component ignores complex computed properties 1`] = ` +Object { + "complex_prop": Object { + "required": true, + "type": Object {}, + }, +} +`; diff --git a/src/handlers/__tests__/componentMethodsHandler-test.js b/src/handlers/__tests__/componentMethodsHandler-test.js index 60eb89d8498..4612d9af7dc 100644 --- a/src/handlers/__tests__/componentMethodsHandler-test.js +++ b/src/handlers/__tests__/componentMethodsHandler-test.js @@ -130,6 +130,35 @@ describe('componentMethodsHandler', () => { test(parse(src).get('body', 0)); }); + it('should handle and ignore computed methods', () => { + const src = ` + class Test { + /** + * The foo method + */ + [foo](bar: number): number { + return bar; + } + + /** + * Should not show up + */ + [() => {}](bar: number): number { + return bar; + } + + componentDidMount() {} + + render() { + return null; + } + } + `; + + componentMethodsHandler(documentation, parse(src).get('body', 0)); + expect(documentation.methods).toMatchSnapshot(); + }); + it('should not find methods for stateless components', () => { const src = ` (props) => {} diff --git a/src/handlers/__tests__/defaultPropsHandler-test.js b/src/handlers/__tests__/defaultPropsHandler-test.js index 5fd846a76a9..fc9009c8a12 100644 --- a/src/handlers/__tests__/defaultPropsHandler-test.js +++ b/src/handlers/__tests__/defaultPropsHandler-test.js @@ -6,9 +6,6 @@ * */ -/*global jest, describe, beforeEach, it, expect*/ - -jest.disableAutomock(); jest.mock('../../Documentation'); describe('defaultPropsHandler', () => { @@ -22,36 +19,6 @@ describe('defaultPropsHandler', () => { defaultPropsHandler = require('../defaultPropsHandler').default; }); - function test(definition) { - defaultPropsHandler(documentation, definition); - expect(documentation.descriptors).toEqual({ - foo: { - defaultValue: { - value: '"bar"', - computed: false, - }, - }, - bar: { - defaultValue: { - value: '42', - computed: false, - }, - }, - baz: { - defaultValue: { - value: '["foo", "bar"]', - computed: false, - }, - }, - abc: { - defaultValue: { - value: '{xyz: abc.def, 123: 42}', - computed: false, - }, - }, - }); - } - describe('ObjectExpression', () => { it('should find prop default values that are literals', () => { const src = ` @@ -66,7 +33,47 @@ describe('defaultPropsHandler', () => { } }) `; - test(parse(src).get('body', 0, 'expression')); + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'expression'), + ); + expect(documentation.descriptors).toMatchSnapshot(); + }); + + it('handles computed properties', () => { + const src = ` + ({ + getDefaultProps: function() { + return { + foo: "bar", + [bar]: 42, + }; + } + }) + `; + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'expression'), + ); + expect(documentation.descriptors).toMatchSnapshot(); + }); + + it('ignores complex computed properties', () => { + const src = ` + ({ + getDefaultProps: function() { + return { + foo: "bar", + [() => {}]: 42, + }; + } + }) + `; + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'expression'), + ); + expect(documentation.descriptors).toMatchSnapshot(); }); }); @@ -82,7 +89,8 @@ describe('defaultPropsHandler', () => { }; } `; - test(parse(src).get('body', 0)); + defaultPropsHandler(documentation, parse(src).get('body', 0)); + expect(documentation.descriptors).toMatchSnapshot(); }); it('should find prop default values that are imported variables', () => { @@ -96,14 +104,7 @@ describe('defaultPropsHandler', () => { } `; defaultPropsHandler(documentation, parse(src).get('body', 1)); - expect(documentation.descriptors).toEqual({ - foo: { - defaultValue: { - value: 'ImportedComponent', - computed: true, - }, - }, - }); + expect(documentation.descriptors).toMatchSnapshot(); }); }); @@ -118,7 +119,11 @@ describe('defaultPropsHandler', () => { abc: {xyz: abc.def, 123: 42} }; }`; - test(parse(src).get('body', 0, 'declarations', 0, 'init')); + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'declarations', 0, 'init'), + ); + expect(documentation.descriptors).toMatchSnapshot(); }); }); @@ -135,14 +140,7 @@ describe('defaultPropsHandler', () => { `; const definition = parse(src).get('body', 0, 'expression'); expect(() => defaultPropsHandler(documentation, definition)).not.toThrow(); - expect(documentation.descriptors).toEqual({ - bar: { - defaultValue: { - value: '42', - computed: false, - }, - }, - }); + expect(documentation.descriptors).toMatchSnapshot(); }); describe('Functional components with default params', () => { @@ -155,7 +153,11 @@ describe('defaultPropsHandler', () => { abc = {xyz: abc.def, 123: 42} }) =>
`; - test(parse(src).get('body', 0, 'expression')); + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'expression'), + ); + expect(documentation.descriptors).toMatchSnapshot(); }); it('should override with defaultProps if available', () => { @@ -168,7 +170,11 @@ describe('defaultPropsHandler', () => { }) =>
Foo.defaultProps = { abc: {xyz: abc.def, 123: 42} }; `; - test(parse(src).get('body', 0, 'declarations', 0, 'init')); + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'declarations', 0, 'init'), + ); + expect(documentation.descriptors).toMatchSnapshot(); }); it('should work with aliases', () => { @@ -180,7 +186,11 @@ describe('defaultPropsHandler', () => { abc: defg = {xyz: abc.def, 123: 42} }) =>
`; - test(parse(src).get('body', 0, 'expression')); + defaultPropsHandler( + documentation, + parse(src).get('body', 0, 'expression'), + ); + expect(documentation.descriptors).toMatchSnapshot(); }); it('should find prop default values that are imported variables', () => { @@ -195,15 +205,7 @@ describe('defaultPropsHandler', () => { documentation, parse(src).get('body', 1, 'expression'), ); - - expect(documentation.descriptors).toEqual({ - foo: { - defaultValue: { - value: 'ImportedComponent', - computed: true, - }, - }, - }); + expect(documentation.descriptors).toMatchSnapshot(); }); it('should work with no defaults', () => { @@ -214,7 +216,7 @@ describe('defaultPropsHandler', () => { documentation, parse(src).get('body', 0, 'expression'), ); - expect(documentation.descriptors).toEqual({}); + expect(documentation.descriptors).toMatchSnapshot(); }); }); }); diff --git a/src/handlers/__tests__/flowTypeHandler-test.js b/src/handlers/__tests__/flowTypeHandler-test.js index f7fa538ebc8..8b12f47c175 100644 --- a/src/handlers/__tests__/flowTypeHandler-test.js +++ b/src/handlers/__tests__/flowTypeHandler-test.js @@ -94,6 +94,20 @@ describe('flowTypeHandler', () => { }); }); + it('ignores hash map entry', () => { + const flowTypesSrc = ` + { + [key: string]: string, + bar?: number, + } + `; + const definition = getSrc(flowTypesSrc); + + flowTypeHandler(documentation, definition); + + expect(documentation.descriptors).toMatchSnapshot(); + }); + it('detects union types', () => { const flowTypesSrc = ` { diff --git a/src/handlers/__tests__/propTypeHandler-test.js b/src/handlers/__tests__/propTypeHandler-test.js index aabba056874..7c34d1b0993 100644 --- a/src/handlers/__tests__/propTypeHandler-test.js +++ b/src/handlers/__tests__/propTypeHandler-test.js @@ -6,8 +6,6 @@ * */ -/*global jest, describe, it, expect, beforeEach*/ - jest.mock('../../Documentation'); jest.mock('../../utils/getPropType', () => jest.fn(() => ({}))); @@ -121,6 +119,36 @@ describe('propTypeHandler', () => { }); }); + it('handles computed properties', () => { + const definition = parse( + getSrc( + `{ + [foo]: PropTypes.array.isRequired, + complex_prop: + PropTypes.oneOfType([PropTypes.number, PropTypes.bool]).isRequired, + }`, + ), + ); + + propTypeHandler(documentation, definition); + expect(documentation.descriptors).toMatchSnapshot(); + }); + + it('ignores complex computed properties', () => { + const definition = parse( + getSrc( + `{ + [() => {}]: PropTypes.array.isRequired, + complex_prop: + PropTypes.oneOfType([PropTypes.number, PropTypes.bool]).isRequired, + }`, + ), + ); + + propTypeHandler(documentation, definition); + expect(documentation.descriptors).toMatchSnapshot(); + }); + it('only considers definitions from React or ReactPropTypes', () => { const definition = parse( getSrc( diff --git a/src/handlers/componentMethodsHandler.js b/src/handlers/componentMethodsHandler.js index 1a695063eee..c7d1ad392a0 100644 --- a/src/handlers/componentMethodsHandler.js +++ b/src/handlers/componentMethodsHandler.js @@ -67,5 +67,8 @@ export default function componentMethodsHandler( } } - documentation.set('methods', methodPaths.map(getMethodDocumentation)); + documentation.set( + 'methods', + methodPaths.map(getMethodDocumentation).filter(Boolean), + ); } diff --git a/src/handlers/defaultPropsHandler.js b/src/handlers/defaultPropsHandler.js index ab452e4267d..1988758a121 100644 --- a/src/handlers/defaultPropsHandler.js +++ b/src/handlers/defaultPropsHandler.js @@ -96,10 +96,11 @@ function getDefaultValuesFromProps( !isStateless || types.AssignmentPattern.check(propertyPath.get('value').node), ) - .forEach(function(propertyPath) { - const propDescriptor = documentation.getPropDescriptor( - getPropertyName(propertyPath), - ); + .forEach(propertyPath => { + const propName = getPropertyName(propertyPath); + if (!propName) return; + + const propDescriptor = documentation.getPropDescriptor(propName); const defaultValue = getDefaultValue( isStateless ? propertyPath.get('value', 'right') diff --git a/src/handlers/flowTypeHandler.js b/src/handlers/flowTypeHandler.js index cd73311c1bf..24776329934 100644 --- a/src/handlers/flowTypeHandler.js +++ b/src/handlers/flowTypeHandler.js @@ -46,9 +46,10 @@ function setPropDescriptor(documentation: Documentation, path: NodePath): void { } } else if (types.ObjectTypeProperty.check(path.node)) { const type = getFlowType(path.get('value')); - const propDescriptor = documentation.getPropDescriptor( - getPropertyName(path), - ); + const propName = getPropertyName(path); + if (!propName) return; + + const propDescriptor = documentation.getPropDescriptor(propName); propDescriptor.required = !path.node.optional; propDescriptor.flowType = type; diff --git a/src/handlers/propTypeHandler.js b/src/handlers/propTypeHandler.js index 33b9682bf9c..9356efff71b 100644 --- a/src/handlers/propTypeHandler.js +++ b/src/handlers/propTypeHandler.js @@ -36,10 +36,13 @@ function amendPropTypes(getDescriptor, path) { return; } - path.get('properties').each(function(propertyPath) { + path.get('properties').each(propertyPath => { switch (propertyPath.node.type) { case types.Property.name: { - const propDescriptor = getDescriptor(getPropertyName(propertyPath)); + const propName = getPropertyName(propertyPath); + if (!propName) return; + + const propDescriptor = getDescriptor(propName); const valuePath = propertyPath.get('value'); const type = isPropTypesExpression(valuePath) ? getPropType(valuePath) diff --git a/src/utils/__tests__/__snapshots__/getMethodDocumentation-test.js.snap b/src/utils/__tests__/__snapshots__/getMethodDocumentation-test.js.snap new file mode 100644 index 00000000000..3a95cebf940 --- /dev/null +++ b/src/utils/__tests__/__snapshots__/getMethodDocumentation-test.js.snap @@ -0,0 +1,13 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`getMethodDocumentation name handles computed method name 1`] = ` +Object { + "docblock": null, + "modifiers": Array [], + "name": "@computed#foo", + "params": Array [], + "returns": null, +} +`; + +exports[`getMethodDocumentation name ignores complex computed method name 1`] = `null`; diff --git a/src/utils/__tests__/__snapshots__/getPropType-test.js.snap b/src/utils/__tests__/__snapshots__/getPropType-test.js.snap index 0e2af1e794a..f41957ab084 100644 --- a/src/utils/__tests__/__snapshots__/getPropType-test.js.snap +++ b/src/utils/__tests__/__snapshots__/getPropType-test.js.snap @@ -102,6 +102,34 @@ Object { } `; +exports[`getPropType handles computed properties 1`] = ` +Object { + "name": "exact", + "value": Object { + "@computed#foo": Object { + "name": "string", + "required": true, + }, + "bar": Object { + "name": "bool", + "required": false, + }, + }, +} +`; + +exports[`getPropType ignores complex computed properties 1`] = ` +Object { + "name": "exact", + "value": Object { + "bar": Object { + "name": "bool", + "required": false, + }, + }, +} +`; + exports[`getPropType resolve identifier to their values correctly resolves SpreadElements in arrays 1`] = ` Object { "name": "enum", diff --git a/src/utils/__tests__/getMethodDocumentation-test.js b/src/utils/__tests__/getMethodDocumentation-test.js index d0f6f6d36d1..4f77432d010 100644 --- a/src/utils/__tests__/getMethodDocumentation-test.js +++ b/src/utils/__tests__/getMethodDocumentation-test.js @@ -35,6 +35,26 @@ describe('getMethodDocumentation', () => { params: [], }); }); + + it('handles computed method name', () => { + const def = statement(` + class Foo { + [foo]() {} + } + `); + const method = def.get('body', 'body', 0); + expect(getMethodDocumentation(method)).toMatchSnapshot(); + }); + + it('ignores complex computed method name', () => { + const def = statement(` + class Foo { + [() => {}]() {} + } + `); + const method = def.get('body', 'body', 0); + expect(getMethodDocumentation(method)).toMatchSnapshot(); + }); }); describe('docblock', () => { diff --git a/src/utils/__tests__/getPropType-test.js b/src/utils/__tests__/getPropType-test.js index 41e5b705749..3aaf8400944 100644 --- a/src/utils/__tests__/getPropType-test.js +++ b/src/utils/__tests__/getPropType-test.js @@ -338,4 +338,26 @@ describe('getPropType', () => { ), ).toMatchSnapshot(); }); + + it('handles computed properties', () => { + expect( + getPropType( + expression(`exact({ + [foo]: string.isRequired, + bar: bool + })`), + ), + ).toMatchSnapshot(); + }); + + it('ignores complex computed properties', () => { + expect( + getPropType( + expression(`exact({ + [() => {}]: string.isRequired, + bar: bool + })`), + ), + ).toMatchSnapshot(); + }); }); diff --git a/src/utils/__tests__/getPropertyName-test.js b/src/utils/__tests__/getPropertyName-test.js new file mode 100644 index 00000000000..0404addb8f0 --- /dev/null +++ b/src/utils/__tests__/getPropertyName-test.js @@ -0,0 +1,95 @@ +/* + * 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. + * + */ + +import { parse, expression } from '../../../tests/utils'; + +describe('getPropertyName', () => { + let getPropertyName; + + function parsePath(src) { + const root = parse(src.trim()); + return root.get('body', root.node.body.length - 1, 'expression'); + } + + beforeEach(() => { + getPropertyName = require('../getPropertyName').default; + }); + + it('returns the name for a normal property', () => { + const def = expression('{ foo: 1 }'); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe('foo'); + }); + + it('returns the name of a object type spread property', () => { + const def = expression('(a: { ...foo })'); + const param = def.get('typeAnnotation', 'typeAnnotation', 'properties', 0); + + expect(getPropertyName(param)).toBe('foo'); + }); + + it('creates name for computed properties', () => { + const def = expression('{ [foo]: 21 }'); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe('@computed#foo'); + }); + + it('creates name for computed properties from string', () => { + const def = expression('{ ["foo"]: 21 }'); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe('foo'); + }); + + it('creates name for computed properties from int', () => { + const def = expression('{ [31]: 21 }'); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe('31'); + }); + + it('returns null for computed properties from regex', () => { + const def = expression('{ [/31/]: 21 }'); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe(null); + }); + + it('returns null for to complex computed properties', () => { + const def = expression('{ [() => {}]: 21 }'); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe(null); + }); + + it('resolves simple variables', () => { + const def = parsePath(` + const foo = "name"; + + ({ [foo]: 21 }); + `); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe('name'); + }); + + it('resolves simple member expressions', () => { + const def = parsePath(` + const a = { foo: "name" }; + + ({ [a.foo]: 21 }); + `); + const param = def.get('properties', 0); + + expect(getPropertyName(param)).toBe('name'); + }); +}); diff --git a/src/utils/getMethodDocumentation.js b/src/utils/getMethodDocumentation.js index 71633dac777..1b901ed7932 100644 --- a/src/utils/getMethodDocumentation.js +++ b/src/utils/getMethodDocumentation.js @@ -102,8 +102,10 @@ function getMethodModifiers(methodPath) { export default function getMethodDocumentation( methodPath: NodePath, -): MethodDocumentation { +): ?MethodDocumentation { const name = getPropertyName(methodPath); + if (!name) return null; + const docblock = getDocblock(methodPath); return { diff --git a/src/utils/getPropType.js b/src/utils/getPropType.js index 40cc73c1dc4..2eb4dc97f6f 100644 --- a/src/utils/getPropType.js +++ b/src/utils/getPropType.js @@ -146,6 +146,8 @@ function getPropTypeShapish(name, argumentPath) { return; } + const propertyName = getPropertyName(propertyPath); + if (!propertyName) return; const descriptor: PropDescriptor | PropTypeDescriptor = getPropType( propertyPath.get('value'), ); @@ -154,7 +156,7 @@ function getPropTypeShapish(name, argumentPath) { descriptor.description = docs; } descriptor.required = isRequiredPropType(propertyPath.get('value')); - value[getPropertyName(propertyPath)] = descriptor; + value[propertyName] = descriptor; }); type.value = value; } diff --git a/src/utils/getPropertyName.js b/src/utils/getPropertyName.js index 6d2c3724e63..595591a848d 100644 --- a/src/utils/getPropertyName.js +++ b/src/utils/getPropertyName.js @@ -9,21 +9,53 @@ import recast from 'recast'; import getNameOrValue from './getNameOrValue'; +import resolveToValue from './resolveToValue'; const { types: { namedTypes: types }, } = recast; +export const COMPUTED_PREFIX = '@computed#'; + /** * In an ObjectExpression, the name of a property can either be an identifier * or a literal (or dynamic, but we don't support those). This function simply * returns the value of the literal or name of the identifier. */ -export default function getPropertyName(propertyPath: NodePath): string { +export default function getPropertyName(propertyPath: NodePath): ?string { if (types.ObjectTypeSpreadProperty.check(propertyPath.node)) { return getNameOrValue(propertyPath.get('argument').get('id'), false); } else if (propertyPath.node.computed) { - throw new TypeError('Property name must be an Identifier or a Literal'); + const key = propertyPath.get('key'); + + // Try to resolve variables and member expressions + if ( + types.Identifier.check(key.node) || + types.MemberExpression.check(key.node) + ) { + const value = resolveToValue(key).node; + + if ( + types.Literal.check(value) && + (typeof value.value === 'string' || typeof value.value === 'number') + ) { + return `${value.value}`; + } + } + + // generate name for identifier + if (types.Identifier.check(key.node)) { + return `${COMPUTED_PREFIX}${key.node.name}`; + } + + if ( + types.Literal.check(key.node) && + (typeof key.node.value === 'string' || typeof key.node.value === 'number') + ) { + return `${key.node.value}`; + } + + return null; } return getNameOrValue(propertyPath.get('key'), false); diff --git a/src/utils/isReactComponentMethod.js b/src/utils/isReactComponentMethod.js index b50d193138a..c4b2a4fcc78 100644 --- a/src/utils/isReactComponentMethod.js +++ b/src/utils/isReactComponentMethod.js @@ -49,5 +49,5 @@ export default function(methodPath: NodePath): boolean { } const name = getPropertyName(methodPath); - return componentMethods.indexOf(name) !== -1; + return !!name && componentMethods.indexOf(name) !== -1; } diff --git a/src/utils/setPropDescription.js b/src/utils/setPropDescription.js index 41915352f73..5e39bfd98d0 100644 --- a/src/utils/setPropDescription.js +++ b/src/utils/setPropDescription.js @@ -11,16 +11,12 @@ import type Documentation from '../Documentation'; import getPropertyName from './getPropertyName'; import { getDocblock } from './docblock'; -/** - * - */ export default (documentation: Documentation, propertyPath: NodePath) => { const propName = getPropertyName(propertyPath); - const propDescriptor = documentation.getPropDescriptor(propName); + if (!propName) return; - if (propDescriptor.description) { - return; - } + const propDescriptor = documentation.getPropDescriptor(propName); + if (propDescriptor.description) return; propDescriptor.description = getDocblock(propertyPath) || ''; };