Skip to content

Fix intersection and union flow types #118

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 18, 2016
Merged

Fix intersection and union flow types #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fe2f43
Refactor for flow intersection and union Props
palange Jul 8, 2016
d95528d
Enable no-undef and no-unused-variables eslint rules
danez Aug 18, 2016
724c919
Do not throw if union type encountered, but silently ignore
danez Aug 18, 2016
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
7 changes: 5 additions & 2 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@
"comma-dangle": [1, "always-multiline"],
"no-underscore-dangle": 0,
"quotes": [2, "single", "avoid-escape"],
"strict": 0
"strict": 0,
"no-unused-vars": 2,
"no-undef": 2
},
"env": {
"node": true
"node": true,
"es6": true
},
"globals": {
"ASTNode": true,
Expand Down
1 change: 1 addition & 0 deletions flow/react-docgen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type FlowTypeDescriptor = {
elements?: Array<FlowTypeDescriptor>,
type?: 'object' | 'function',
signature?: flowObjectSignatureType | flowFunctionSignatureType,
alias?: string,
};

type PropDescriptor = {
Expand Down
46 changes: 46 additions & 0 deletions src/handlers/__tests__/flowTypeDocBlockHandler-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,52 @@ describe('flowTypeDocBlockHandler', () => {
.not.toThrow();
});

it('supports intersection proptypes', () => {
var definition = statement(`
(props: Props) => <div />;

var React = require('React');
import type Imported from 'something';

type BarProps = {
/** bar description */
bar: 'barValue'
}

type Props = Imported & BarProps & {
/** foo description */
foo: 'fooValue'
};
`).get('expression');

flowTypeDocBlockHandler(documentation, definition);

expect(documentation.descriptors).toEqual({
foo: {
description: 'foo description',
},
bar: {
description: 'bar description',
},
});

it('does not support union proptypes', () => {
var definition = statement(`
(props: Props) => <div />;

var React = require('React');
import type Imported from 'something';

type Other = { bar: 'barValue' };
type Props = Imported | Other | { foo: 'fooValue' };
`).get('expression');

expect(() => flowTypeDocBlockHandler(documentation, definition))
.toThrowError(TypeError, "react-docgen doesn't support Props of union types");
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this not throw as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch. The test wasn't executed because of wrong nesting. Fixed it.

});
});


describe('does not error for unreachable type', () => {
function test(code) {
var definition = statement(code).get('expression');
Expand Down
22 changes: 9 additions & 13 deletions src/handlers/__tests__/flowTypeHandler-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,13 @@ describe('flowTypeHandler', () => {
it('supports intersection proptypes', () => {
var definition = statement(`
(props: Props) => <div />;

var React = require('React');
import type Imported from 'something';

type Props = Imported & { foo: 'bar' };
`).get('expression');


flowTypeHandler(documentation, definition);

expect(documentation.descriptors).toEqual({
Expand All @@ -188,24 +187,21 @@ describe('flowTypeHandler', () => {
});
});

it('supports union proptypes', () => {
it('does not support union proptypes', () => {
var definition = statement(`
(props: Props) => <div />;

var React = require('React');
import type Imported from 'something';

type Props = Imported | { foo: 'bar' };
type Other = { bar: 'barValue' };
type Props = Imported | Other | { foo: 'fooValue' };
`).get('expression');

flowTypeHandler(documentation, definition);
expect(() => flowTypeHandler(documentation, definition))
.not.toThrow();

expect(documentation.descriptors).toEqual({
foo: {
flowType: {},
required: true,
},
});
expect(documentation.descriptors).toEqual({});
});

describe('does not error for unreachable type', () => {
Expand Down
1 change: 0 additions & 1 deletion src/handlers/componentMethodsHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import getMemberValuePath from '../utils/getMemberValuePath';
import getMethodDocumentation from '../utils/getMethodDocumentation';
import isReactComponentClass from '../utils/isReactComponentClass';
import isReactComponentMethod from '../utils/isReactComponentMethod';
import isReactCreateClassCall from '../utils/isReactCreateClassCall';

import type Documentation from '../Documentation';

Expand Down
10 changes: 7 additions & 3 deletions src/handlers/flowTypeDocBlockHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,23 @@

import type Documentation from '../Documentation';
import setPropDescription from '../utils/setPropDescription';
import getFlowTypeFromReactComponent from '../utils/getFlowTypeFromReactComponent';
import getFlowTypeFromReactComponent, {
applyToFlowTypeProperties,
} from '../utils/getFlowTypeFromReactComponent';

/**
* This handler tries to find flow Type annotated react components and extract
* its types to the documentation. It also extracts docblock comments which are
* inlined in the type definition.
*/
export default function flowTypeDocBlockHandler(documentation: Documentation, path: NodePath) {
let flowTypesPath = getFlowTypeFromReactComponent(path);
const flowTypesPath = getFlowTypeFromReactComponent(path);

if (!flowTypesPath) {
return;
}

flowTypesPath.get('properties').each(propertyPath => setPropDescription(documentation, propertyPath));
applyToFlowTypeProperties(flowTypesPath, propertyPath =>
setPropDescription(documentation, propertyPath)
);
}
21 changes: 6 additions & 15 deletions src/handlers/flowTypeHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,30 +14,19 @@ import type Documentation from '../Documentation';

import getFlowType from '../utils/getFlowType';
import getPropertyName from '../utils/getPropertyName';
import getFlowTypeFromReactComponent from '../utils/getFlowTypeFromReactComponent';
import getFlowTypeFromReactComponent, {
applyToFlowTypeProperties,
} from '../utils/getFlowTypeFromReactComponent';

function setPropDescriptor(documentation: Documentation, path: NodePath): void {
const propDescriptor = documentation.getPropDescriptor(getPropertyName(path));
const type = getFlowType(path.get('value'));

if (type) {
propDescriptor.flowType = type;
propDescriptor.required = !path.node.optional;
}
}

function findAndSetTypes(documentation: Documentation, path: NodePath): void {
if (path.node.properties) {
path.get('properties').each(
propertyPath => setPropDescriptor(documentation, propertyPath)
);
} else if (path.node.types) {
path.get('types').each(
typesPath => findAndSetTypes(documentation, typesPath)
);
}
}

/**
* This handler tries to find flow Type annotated react components and extract
* its types to the documentation. It also extracts docblock comments which are
Expand All @@ -50,5 +39,7 @@ export default function flowTypeHandler(documentation: Documentation, path: Node
return;
}

findAndSetTypes(documentation, flowTypesPath);
applyToFlowTypeProperties(flowTypesPath, propertyPath => {
setPropDescriptor(documentation, propertyPath)
});
}
4 changes: 2 additions & 2 deletions src/utils/__tests__/getMethodDocumentation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ jest.disableAutomock();

describe('getMethodDocumentation', () => {
let getMethodDocumentation;
let expression, statement;
let statement;

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

describe('name', () => {
Expand Down
4 changes: 2 additions & 2 deletions src/utils/__tests__/getTypeAnnotation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
jest.disableAutomock();

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

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

it('detects simple type', () => {
Expand Down
44 changes: 42 additions & 2 deletions src/utils/getFlowTypeFromReactComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
*
*/

import getPropertyName from './getPropertyName';
import getTypeAnnotation from '../utils/getTypeAnnotation';
import getMemberValuePath from '../utils/getMemberValuePath';
import isReactComponentClass from '../utils/isReactComponentClass';
Expand Down Expand Up @@ -51,7 +50,48 @@ export default (path: NodePath): ?NodePath => {
if (typePath && types.GenericTypeAnnotation.check(typePath.node)) {
typePath = resolveToValue(typePath.get('id'));
if (
!typePath ||
!typePath ||
types.Identifier.check(typePath.node) ||
isUnreachableFlowType(typePath)
) {
return;
}

typePath = typePath.get('right');
}

return typePath;
}

export function applyToFlowTypeProperties(
path: NodePath,
callback: (propertyPath: NodePath) => void
) {
if (path.node.properties) {
path.get('properties').each(
propertyPath => callback(propertyPath)
);
} else if (path.node.type === 'IntersectionTypeAnnotation') {
path.get('types').each(
typesPath => applyToFlowTypeProperties(typesPath, callback)
);
} else if (path.node.type !== 'UnionTypeAnnotation') {
// The react-docgen output format does not currently allow
// for the expression of union types
Copy link
Member

Choose a reason for hiding this comment

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

What would we have to do to make this work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This depends on how exactly we want to go for this. Quoting the flow docs:

A union type requires for a value to be one of the input types.

/* @flow */
type U = number | string;
var x: U = 1;
x = "two";

An intersection type requires a value to be all of the input types:

/* @flow */
type I = {a: number} & {b: number};
var x: I = {a: 1, b: 2};
x = {a: 1, b: 2, c: "three"};

So intersection was quite straight forward, just merge all objects and the result is the object with all possible props.

With union this is more complex. In this example, one can see that when using union for prop-types it is one of the objects to be "unioned" (with additional properties allowed).

So in the simple case of

 type U = {a: number} | {a: string, b: string};

The resulting docs would need to be: "prop a is required and needs to be a number, or it is a string but then also b is required and needs to be a string"

This might make sense in some react components, but I'm not sure yet how we would document this cases. We maybe could only add the raw value.

P.S.: I might be mistaken here, but I think it should be correct. Never used union and intersect in these cases myself. :)

let typePath = resolveGenericTypeAnnotation(path);
if (typePath) {
applyToFlowTypeProperties(typePath, callback);
}
}
}

function resolveGenericTypeAnnotation(path: NodePath): ?NodePath {
// If the node doesn't have types or properties, try to get the type.
let typePath: ?NodePath;
if (path && types.GenericTypeAnnotation.check(path.node)) {
typePath = resolveToValue(path.get('id'));
if (
!typePath ||
types.Identifier.check(typePath.node) ||
isUnreachableFlowType(typePath)
) {
Expand Down
4 changes: 2 additions & 2 deletions src/utils/getMethodDocumentation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type MethodDocumentation = {
returns: ?MethodReturn;
};

function getMethodParamsDoc(methodPath, jsDoc) {
function getMethodParamsDoc(methodPath) {
const params = [];
const functionExpression = methodPath.get('value');

Expand Down Expand Up @@ -64,7 +64,7 @@ function getMethodParamsDoc(methodPath, jsDoc) {
}

// Extract flow return type.
function getMethodReturnDoc(methodPath, jsDoc) {
function getMethodReturnDoc(methodPath) {
const functionExpression = methodPath.get('value');

if (functionExpression.node.returnType) {
Expand Down