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

fix(Documentation, prop-types) Problems with propTypes. #1377

Open
6 tasks
fnpen opened this issue Jan 23, 2019 · 5 comments
Open
6 tasks

fix(Documentation, prop-types) Problems with propTypes. #1377

fnpen opened this issue Jan 23, 2019 · 5 comments

Comments

@fnpen
Copy link

fnpen commented Jan 23, 2019

Issue description

Many components have actual descriptions and props(but defined in parent component).

Errors in tests:
image

Documentation mistakes.

Suggestions

  • Correct propTypes to spread prop-types of parent (correct type-checking on runtime)
  • Move comments from docs to component prop-types
Component.propTypes = {
    ... ParentComponent.propTypes,
    /** Comment */
    group: PropTypes.bool,
}

If we will have correct prop-types and descriptions in one place, it will help us support and understand library better in future.

Do you need pull-request?

@TheSharpieOne
Copy link
Member

Yes, please‼️

Docs are in bad shape based on all of the feedback, but with the limited time I have, they are the last thing on the list (since they don't enhance or fix the library itself).
I would love for someone to redo the docs to make them easier to maintain and just all-around better. I personally learn by examples which is one of the reasons why the docs are basically just a bunch of examples. I know not everyone is like that, so I welcome someone else to improve them.

@fnpen fnpen closed this as completed Jan 25, 2019
@fnpen fnpen reopened this Jan 25, 2019
@fnpen
Copy link
Author

fnpen commented Jan 25, 2019

@TheSharpieOne I will start this task.

@fnpen
Copy link
Author

fnpen commented Jan 27, 2019

@TheSharpieOne
I'm thinking about variants to read propTypes, default values and their descriptions and tried many variants.

1. Use react-docgen via babel and webpack-loader.
Working nice, but parse files statically and do not analyze imported propTypes, like:

import Alert from './Alert';

SecondAlert = {
    ...Alert.propTypes,
};

Will show props declared in the current file.

2. Use runtime runs to extract via NodeJS to generate JSON with all meta, via prop-type mutation.
Will show all propType also from a parent, but we can't read descriptions from comments(need static analysis and difficult to merge them).

extract-types.js

import Alert from './src/Alert.js';
processTypes(Alert);

but we need proxy prop-types.

And we need to read the description from class:

Alert.propDescriptions = {
    label: 'Label'
}

3. Use TypeScript.
Write .d.ts for each components file with props, descriptions and parse it via typedoc
And read default values runtime.

Overall:

  1. is not universal, partial support of functions.
  2. good, but we need descriptions.
  3. best, we can generate @types inside repo and generate props table in the documentation.

The third option is better, but need more time and will solve many problems. Other ways are hacky or partial. I like the third.

@TheSharpieOne
Copy link
Member

TheSharpieOne commented Jan 29, 2019

Adding .d.ts seems like the same problem: having two difference places to define the props. We just move it from the docs itself to somewhere else to generate the docs.

I wish the first option was better. Not only does it not like spread, but it also don't like propTypes from any imported files. We define a couple prop types in a common file and just import them where needed rather than define them everywhere (such as DOMElement and tag), those would have problems with the first option as well (according to reactjs/react-docgen#33).
Maybe something like https://github.com/benjroy/react-docgen-imported-proptype-handler can fix the imported part and a comment in the docs could be good enough for the spread.

I suppose adding .d.ts in the project would be a good thing as long as it stays up to date with the actual components.

@fnpen
Copy link
Author

fnpen commented Jan 30, 2019

Now I'm working on the first way.
I agree, the third way is more difficult to do and harder to work for other contributors because they need to edit two types of files,
and I tried "react-docgen-imported-proptype-handler," and it is throwing an error with this repo. I wrote the plugin to parse imported propTypes, and I need to parse "deprecated" wrapper and imported types.

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

No branches or pull requests

2 participants