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): allow parsing typescript files #11265

Merged
merged 3 commits into from
Jan 29, 2019

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jan 24, 2019

Typescript has no mime type :/

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Updated some tests to a little less janky (not your doing, they were already janky!), and then left a few comments.

This looks good though, thanks!

@@ -10,6 +10,19 @@ const digest = str =>
const propsId = (parentId, name) => `${parentId}--ComponentProp-${name}`
const descId = parentId => `${parentId}--ComponentDescription`

function canParse(node) {
return (
node &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is really needed!

Suggested change
node &&

Copy link
Contributor

Choose a reason for hiding this comment

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

(although it will be with one of the tests I added, e.g. null node 😩, so that'll have to be fixed when/if this is updated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya it more out of an abundance of caution i can remove if you like?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably OK. Let's just merge this, thank you!

node &&
(node.internal.mediaType === `application/javascript` ||
// Typescript doesn't really have a mime type and .ts files are a media file :/
node.internal.mediaType === `application/typescript` ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we need all of these checks? I would've thought there's one condition we could reliably use, i.e.

return (
  node.internal.mediaType === `application/javascript` ||
  node.internal.mediaType === `application/typescript`
)

Copy link
Contributor Author

@jquense jquense Jan 25, 2019

Choose a reason for hiding this comment

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

The above actually won't work, since mime types has no typescript mime, I added it in case the input node was from another source plugin that does use the likely mime type.

In the case of File nodes we have to check the extension to distinguish the type :/

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looks good, thanks so much Jason!

@DSchau DSchau changed the title feat: react-docgen allow parsing typescript files feat(gatsby-transformer-react-docgen): allow parsing typescript files Jan 29, 2019
@DSchau DSchau merged commit c9a8991 into master Jan 29, 2019
@DSchau DSchau deleted the docgen-typescript branch January 29, 2019 13:59
@DSchau
Copy link
Contributor

DSchau commented Jan 29, 2019

Published as gatsby-transformer-react-docgen@3.0.3

@jquense
Copy link
Contributor Author

jquense commented Jan 29, 2019

🎉

@thchia
Copy link
Contributor

thchia commented Feb 1, 2019

@jquense I'm getting the following error when attempting to parse a TS file:

error There was a problem parsing component metadata for file: "layout.tsx"

   8 | interface LayoutProps {
   9 |   children?: React.ReactNode
> 10 |   foo: string
     |  ^
  11 | }
  12 |
  13 | /**


  SyntaxError: Unexpected token (10:2)

- index.js:3834 Object.raise
    [gatsby-ts-docgen]/[@babel]/parser/lib/index.js:3834:17

  - index.js:5142 Object.unexpected
    [gatsby-ts-docgen]/[@babel]/parser/lib/index.js:5142:16

  - index.js:1542 Object.flowObjectTypeSemicolon
    [gatsby-ts-docgen]/[@babel]/parser/lib/index.js:1542:12

...

Seems to be related to the flowObjectTypeSemicolon check during the parse operation. AFAIK, omitting the semicolon is valid TS.

@jquense
Copy link
Contributor Author

jquense commented Feb 1, 2019

@thchia yeah at the moment full ts support isn't there, this adds the ability to even see .ts/.tsx files. We still need better upstream react-docgen support, e.g. reactjs/react-docgen#320

@thchia
Copy link
Contributor

thchia commented Feb 1, 2019

@jquense would adding https://github.com/styleguidist/react-docgen-typescript be of any help? I'm playing around now trying to switch parsers based on the file type.

@jquense
Copy link
Contributor Author

jquense commented Feb 1, 2019

I'd really like to not deal with that, and instead just snag the proptypes handler from there and use it with the original react-docgen.

@thchia
Copy link
Contributor

thchia commented Feb 1, 2019

Do you mean not deal with adding an extra dependency? Because it looks like the snaggable bits form the bulk of the code anyway. Either way I'm with you on not having multiple parser instances.

@jquense
Copy link
Contributor Author

jquense commented Feb 1, 2019

it's a few things, not having the extra dep but also not having to deal with inconsistencies and differences between the two tools.

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.

3 participants