Skip to content

[flow] Ignore spread properties in object type #339

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

Merged
merged 1 commit into from
Apr 5, 2019
Merged

Conversation

fkling
Copy link
Member

@fkling fkling commented Apr 4, 2019

The new fixtures shows an example that currently fails with the error

TypeError: Argument must be an Identifier or a Literal
    at getNameOrValue (/home/fkling/git/react-docgen/dist/utils/getNameOrValue.js:40:13)
    at getPropertyName (/home/fkling/git/react-docgen/dist/utils/getPropertyName.js:34:40)
    at NodePath.path.get.each.param (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:171:41)
    at NodePath.each (/home/fkling/git/react-docgen/node_modules/ast-types/lib/path.js:89:26)
    at Object.handleObjectTypeAnnotation [as ObjectTypeAnnotation] (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:168:26)
    at getFlowTypeWithResolvedTypes (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:274:35)
    at Object.handleGenericTypeAnnotation [as GenericTypeAnnotation] (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:143:14)
    at getFlowTypeWithResolvedTypes (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:274:35)
    at getFlowType (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:305:16)
    at NodePath.functionExpression.get.each.paramPath (/home/fkling/git/react-docgen/dist/utils/getMethodDocumentation.js:43:39)

That's because the method is referencing the Props type, but is not setup to
deal with spread properties (like the flowTypeHandler) is.

This change updates the code to ignore anything that is not a property.

We could also change this to resolve spread properties locally and merge them, like the handler does, but I wasn't quite sure how to do that (the handler seems to work quite differently).

The new fixtures shows an example that currently fails with the error

```
TypeError: Argument must be an Identifier or a Literal
    at getNameOrValue (/home/fkling/git/react-docgen/dist/utils/getNameOrValue.js:40:13)
    at getPropertyName (/home/fkling/git/react-docgen/dist/utils/getPropertyName.js:34:40)
    at NodePath.path.get.each.param (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:171:41)
    at NodePath.each (/home/fkling/git/react-docgen/node_modules/ast-types/lib/path.js:89:26)
    at Object.handleObjectTypeAnnotation [as ObjectTypeAnnotation] (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:168:26)
    at getFlowTypeWithResolvedTypes (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:274:35)
    at Object.handleGenericTypeAnnotation [as GenericTypeAnnotation] (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:143:14)
    at getFlowTypeWithResolvedTypes (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:274:35)
    at getFlowType (/home/fkling/git/react-docgen/dist/utils/getFlowType.js:305:16)
    at NodePath.functionExpression.get.each.paramPath (/home/fkling/git/react-docgen/dist/utils/getMethodDocumentation.js:43:39)
```

That's because the method is referencing the `Props` type, but is not setup to
deal with spread properties (like the flowTypeHandler) is.

This change adds similar logic to the object type resolver.
@fkling fkling changed the title [flow] Resolve spread properties in object type [flow] Ignore spread properties in object type Apr 4, 2019
@fkling fkling requested a review from danez April 4, 2019 23:16
@fkling fkling merged commit 48dda4d into master Apr 5, 2019
@fkling fkling deleted the flow_object_spread branch April 5, 2019 00:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants