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

doc update for typescript props config #1514

Merged
merged 6 commits into from
Feb 10, 2020

Conversation

thecodejack
Copy link
Contributor

documentation update for #1508

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks!

docs/Thirdparties.md Outdated Show resolved Hide resolved
Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Looks like I wrote a comment here a long time ago and didn't send it ;-/

@@ -54,6 +54,20 @@ module.exports = {

Please see our [examples](https://github.com/styleguidist/react-styleguidist/tree/master/examples) and refer to [react-docgen](https://github.com/reactjs/react-docgen) documentation for more information about what types of syntax are supported.

But if components are written with typescript and extending other components (which are also written using typescript), You may use `react-docgen-typescript` module for fetching extended component prop definitions automatically. Follow the steps to configure same.
Copy link
Member

Choose a reason for hiding this comment

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

Still confusing for me: what exactly isn't working — extension or import from another file? A code example would be very good here too.

Copy link
Contributor Author

@thecodejack thecodejack Jan 26, 2020

Choose a reason for hiding this comment

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

context was set in before para. Example of how it doesn't work probably unrelated.

Copy link
Member

Choose a reason for hiding this comment

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

It is very much "related". I still don't understand what's the problem exactly, and, looking at the comments, you probably don't understand either. We can't expect users to understand it.

Copy link
Contributor Author

@thecodejack thecodejack Feb 1, 2020

Choose a reason for hiding this comment

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

Have done the changes. Sorry for resolving bit late.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but it still doesn't answer my question: what exactly isn't working — importing any component or specifically from node_modules. From what you've said before I understood it's the former, but from the text it looks like the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually issue seems to be from both any component and node_modules. React doc gen doesn't support. But with react-docgen-typescript, both will work.

Copy link
Member

Choose a reason for hiding this comment

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

So something like this also won't work?

import Button from './AnotherButton'
export default Button

Copy link
Contributor Author

@thecodejack thecodejack Feb 6, 2020

Choose a reason for hiding this comment

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

My bad. I was initially discussing with what I am aware of. So I spent sometime around this. So here is a detailed info.

First it's surely not working with extended third party components.

Example: Please check #1508 (comment)

Second, components which are extended from internal ones are working fine.

Example: https://github.com/styleguidist/react-styleguidist/tree/11/examples/styled-components/src/components/Heading

But I was checking wrong example which didn't work.

Example: https://github.com/styleguidist/react-styleguidist/tree/11/examples/styled-components/src/components/Button

This one didn't work most probably coz it is using style component declaration which is not a standard React component creating pattern.

Now checked above directly using react-docgen-typescript as well. Apart from displaying the 3 declared props, it also displayed props of styled component which is again 3rd party component.

There are lot of ifs/buts around this. But what I can summarize from this, is to inform user that for fetching props of third party components only, we should recommend users to use react-docgen-typescript.

Let know your thoughts around this.

Copy link
Member

Choose a reason for hiding this comment

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

First it's surely not working with extended third party components.

That's clear now, but let's not call then "third-party", since it might be ambiguous. "Components rexeported from node_modules" would be more precise.

Second, components which are extended from internal ones are working fine.
Example: https://github.com/styleguidist/react-styleguidist/tree/11/examples/styled-components/src/components/Heading

This component doesn't extend another component.

Now checked above directly using react-docgen-typescript as well. Apart from displaying the 3 declared props, it also displayed props of styled component which is again 3rd party component.

There's a way to filter props in react-docgen-typescript:

https://github.com/tamiadev/tamia/blob/038390016cbe62480dc385c1656c6f5b4e1fe53f/styleguide.config.js#L10-L18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component doesn't extend another component.

index.ts of Heading Component is importing component from Heading.tsx. So something like below will also work.

import Button from './AnotherButton'
export default Button

Have done the changes as suggested by you.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

I thinks something like this may work.

@@ -54,6 +54,28 @@ module.exports = {

Please see our [examples](https://github.com/styleguidist/react-styleguidist/tree/master/examples) and refer to [react-docgen](https://github.com/reactjs/react-docgen) documentation for more information about what types of syntax are supported.

But while writing components using typescript and re-exporting a component from node_modules, You can still be able to fetch prop definitions of the parent component using `react-docgen-typescript`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
But while writing components using typescript and re-exporting a component from node_modules, You can still be able to fetch prop definitions of the parent component using `react-docgen-typescript`.
While Styleguidist supports TypeScript out of the box, thanks to `react-docgen`, this support is limited. Consider this example:```

export default Button
```

In the above example, we were trying to extend a component which is from node_modules. Now if we want prop definitions of the parent component, `react-docgen` won't be able to fetch them. For same you can setup `react-docgen-typescript` with `styleguidist` by following below steps.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In the above example, we were trying to extend a component which is from node_modules. Now if we want prop definitions of the parent component, `react-docgen` won't be able to fetch them. For same you can setup `react-docgen-typescript` with `styleguidist` by following below steps.
Here we’re reexporting a third-party component from `node_modules`. Styleguidist wont be able to render prop types of this component, unless we’re using `react-docgen-typescript`:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Suggestions totally right. Updated with suggestions.

@sapegin sapegin merged commit cb3c00f into styleguidist:11 Feb 10, 2020
@sapegin
Copy link
Member

sapegin commented Feb 10, 2020

Thanks, merging!

@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants