Skip to content
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

feat(gatsby-transformer-react-docgen): fallback to TypeScript or Flow #15202

Merged
merged 8 commits into from
Aug 9, 2019

Conversation

svenliebig
Copy link
Contributor

@svenliebig svenliebig commented Jun 28, 2019

The key type on the property is undefined when parsing typescript files.

Example TypeScript file that caused this issue:

import React, { ReactNode } from "react"

export type Props = {
    anyThing: string
    /**
     *
     * @type {ReactNode}
     * @memberof Props
     */
    children: ReactNode
}

export default function Anything({  }: Props) {
    return <div>content</div>
}

That was because the react-docgen parser sets the flowType key instead of type when no JSDoc is written.

Error is:

error There was a problem parsing component metadata for file:


  TypeError: Cannot set property 'name' of undefined
  
  - doclets.js:77 prop.doclets.forEach
    [components-documentation]/[gatsby-transformer-react-docge
    n]/doclets.js:77:28
  
  - Array.forEach
  
  - doclets.js:71 applyPropDoclets
    [components-documentation]/[gatsby-transformer-react-docge
    n]/doclets.js:71:18
  
  - parse.js:74 component.props.Object.keys.map.propName
    [components-documentation]/[gatsby-transformer-react-docge
    n]/parse.js:74:37
  
  - Array.map
  
  - parse.js:68 components.forEach.component
    [components-documentation]/[gatsby-transformer-react-docge
    n]/parse.js:68:58
  
  - Array.forEach
  
  - parse.js:64 parseMetadata
    [components-documentation]/[gatsby-transformer-react-docge
    n]/parse.js:64:14

The key `type` on the `property` is undefined when parsing `typescript` files.

Example TypeScript file that caused this issue:

```tsx
import React, { ReactNode } from "react"

export type Props = {
    anyThing: string
    /**
     *
     * @type {ReactNode}
     * @memberof Props
     */
    children: ReactNode
}

export default function Anything({  }: Props) {
    return <div>content</div>
}
```

That was because the react-docgen parser sets the `flowType` key instead of `type` when no JSDoc is written.
@svenliebig svenliebig requested a review from a team as a code owner June 28, 2019 08:39
@svenliebig
Copy link
Contributor Author

This hole thing gets fixed in react-docgen@5.0.0-beta.1

It is possible to use it in your current projects with:

"resolutions": {
        "react-docgen": "5.0.0-beta.1"
}

Closing this PR - since this fix is a suitable workaround

@svenliebig svenliebig closed this Jun 28, 2019
@svenliebig
Copy link
Contributor Author

correction, its not fixed when you use documentation in code.

@svenliebig svenliebig reopened this Jun 28, 2019
@svenliebig
Copy link
Contributor Author

I parsed this test interface:

export interface Props {
    /**
     * Perfectly Documented
     *
     * @type {string}
     * @memberof Props
     */
    color: string

    /**
     * Bad Documented
     * @memberof Props
     */
    children: ReactNode

    // Nearly not Documented
    type: string

    notDocument: number | any
}

Results

Good Doc

/**
 * Perfectly Documented
 *
 * @type {string}
 * @memberof Props
 */
color: string

Parsed Ouput

  • tsType is set because of TypeScript
  • type is set because of the documentation
  • flowType is null because the ist no flow
{
    "tsType": {
        "name": "string"
    },
    "type": {
        "name": "string"
    },
    "name": "color",
    "required": true,
    "docblock": "Perfectly Documented\n\n@type {string}\n@memberof Props",
    "description": {
    "text": "Perfectly Documented"
    },
    "flowType": null,
    "doclets": [
        {
            "tag": "type",
            "value": "{string}"
        },
        {
            "tag": "memberof",
            "value": "Props"
        }
    ]
}

Bad Doc

/**
 * Bad Documented
 * @memberof Props
 */
children: ReactNode

Result

  • tsType is set because of TypeScript
  • type is null because of the missing documentation
  • flowType is null because the ist no flow
{
    "tsType": {
        "name": "ReactNode"
    },
    "type": null,
    "name": "children",
    "required": true,
    "docblock": "Bad Documented\n@memberof Props",
    "description": {
        "text": "Bad Documented"
    },
    "flowType": null,
    "doclets": [
        {
            "tag": "memberof",
            "value": "Props"
        }
    ]
}

since that i guess the priority of defining type if it is undefined should be:

{ ...prop.tsType } || { ...prop.flowType } || {}

@KyleAMathews
Copy link
Contributor

Could you add a new test for this? Thanks for the contribution!

@wardpeet wardpeet self-assigned this Jul 29, 2019
@svenliebig
Copy link
Contributor Author

Hey @KyleAMathews i added some tests to the use case, actually found a little bug.
Thanks for your time!

@wardpeet
Copy link
Contributor

that's awesome! :) I was going to look into this one, thanks for getting back to us! Is it possible to test unions with tsType & flowType too? It seems like they are handled a bit different than prop types.

@wardpeet wardpeet changed the title add an undefined check feat(gatsby-transformer-react-docgen): fallback to ts or flow type when propType is not set. Jul 30, 2019
@wardpeet wardpeet changed the title feat(gatsby-transformer-react-docgen): fallback to ts or flow type when propType is not set. feat(gatsby-transformer-react-docgen): fallback to ts or flow type when type is not set. Jul 30, 2019
@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Jul 30, 2019
@svenliebig
Copy link
Contributor Author

Hey @wardpeet! Thanks for answering, i actually tried out many constructions with typeScript now and thought it would be useful to have some doclets tests too, i added them. My working examples for the type tests were:

/**
 * @type {React.Component<Props>}
 */
component: React.Component<Props>
/**
 * @type {(number | any)}
 */
union: number | any

I looked for the default behauvior thats implemented, found a little bug in the doclet parsing when this string is converterted: "(number | any)" the two values came up as "number " and " any" with whitespaces, trimmed these and added a test. And much more tests for unions.

@wardpeet
Copy link
Contributor

wardpeet commented Aug 1, 2019

Thank you. Any chance you can create a small reproduction on how to test this PR?

@svenliebig
Copy link
Contributor Author

I have a current project where i use it:
https://github.com/Sly321/gatsby-doc-yaml

You have to use yarn to build it, because it's mandatory to use react-docgen": "5.0.0-beta.1 as resolution version for this package. They had a bug.

I changed this package (transformer-react-docgen) manually, when you update it to this PR you can just gatsby develop and lookup in the graphql api or the in pages/components/button.js file.

The parsed file is /library/content

@sidharthachatterjee sidharthachatterjee added status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged and removed status: awaiting author response Additional information has been requested from the author labels Aug 9, 2019
@sidharthachatterjee
Copy link
Contributor

Since this depends on behaviour in the v5 beta, I’ll merge this in after #16094 is published (major version with breaking change and upgraded to v5) which will likely be later today

@sidharthachatterjee sidharthachatterjee removed the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Aug 9, 2019
@sidharthachatterjee sidharthachatterjee changed the title feat(gatsby-transformer-react-docgen): fallback to ts or flow type when type is not set. feat(gatsby-transformer-react-docgen): fallback to TypeScript or Flow Aug 9, 2019
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Good to go in now! Thank you so much @Sly321 👍

@sidharthachatterjee sidharthachatterjee merged commit 67c0e3b into gatsbyjs:master Aug 9, 2019
@gatsbot
Copy link

gatsbot bot commented Aug 9, 2019

Holy buckets, @Sly321 — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants