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

Add support for imported propTypes #33

Closed
echenley opened this issue Oct 19, 2015 · 50 comments
Closed

Add support for imported propTypes #33

echenley opened this issue Oct 19, 2015 · 50 comments

Comments

@echenley
Copy link

Importing prop types is a fairly common use case which doesn't appear to be possible at this stage. Any plans to support this pattern?

Example:

// defaultPropTypes.js

import { PropTypes } from 'react';

export default {
    prop1: PropTypes.string
};
// otherPropTypes.js

import { PropTypes } from 'react';

export default {
    prop2: PropTypes.object
};
// MyComponent.jsx

import { Component } from 'react';
import defaultPropTypes from './defaultPropTypes.js';
import otherPropTypes './otherPropTypes.js';

export default class MyComponent extends Component {
    static propTypes = {
        ...defaultPropTypes,
        ...otherPropTypes
    }
    // etc...
}

Results in:

{
  "MyComponent.jsx": {
    "description": ""
  }
}
@fkling
Copy link
Member

fkling commented Oct 20, 2015

I guess we could do this for simple cases, where a variable is spread into propTypes (i.e. ...foo, but not ..foo.bar) and that variable resolves to a module import.

I didn't want to make react-docgen make any assumptions about how module identifiers are resolved, but I guess it's fair to assume that it will be CommonJS compatible (i.e. module identifiers refer to npm modules or local files).

Then we could inspect the other file and if it returns an object literal, inspect and merge it as well.

@frontendphil
Copy link

+1 :)

@oliviertassinari
Copy link

That would be a great feature

@thebuilder
Copy link

I've got a similar usecase, but not using spread when injecting the props.

// ILink.js
import {PropTypes} from 'react';

export default PropTypes.shape({
  url: PropTypes.string.isRequired,
  selected: PropTypes.bool,
  target: PropTypes.string,
  title: PropTypes.string.isRequired
});
// MyComponent.jsx

import { Component } from 'react';
import ILink from './ILink.js';
import otherPropTypes './otherPropTypes.js';

export default class MyComponent extends Component {
    static propTypes = {
        link: ILink
    }
    // etc...
}

@danez
Copy link
Collaborator

danez commented Mar 2, 2016

The same idea would also apply to flow types, as they can be also imported. So maybe this "import something" could be abstracted in a way so it makes imported propTypes and flowTypes possible.

@pasupuletics
Copy link

Even we stuck with similar use case, any plans to add support for external imports of propTypes, below is our use case.

constants.js
const BUTTON_STYLES = ['A', 'B', 'C', 'D'];
export default BUTTON_STYLES;
component.js
import BUTTON_STYLES from 'constants';
propTypes = {
kind: PropTypes.oneOf(constants),
}

class SomeComp extends React.Component{
static propTypes = propTypes;
}

export default SomeComp;
Generated AST:
'kind': {
            'type': {
                'name': 'enum',
                'computed': true,
                'value': 'BUTTON_STYLES'
            },
            'required': false,
            'description': 'some description',
            'defaultValue': {
                'value': 'A',
                'computed': false
            }
        },
Expected AST:

value of AST should be as below(which is working fine when BUTTON_STYLES are in same file, but not with import)

'kind': {
            'type': {
                'name': 'enum',
                'value': [
                    {
                        'value': 'A',
                        'computed': false
                    },
                    {
                        'value': 'B',
                        'computed': false
                    },
                    {
                        'value': 'C',
                        'computed': false
                    },
                    {
                        'value': 'D',
                        'computed': false
                    }
                ]
            },
            'required': false,
            'description': 'Button has a a different set of `kinds`',
            'defaultValue': {
                'value': 'A',
                'computed': false
            }
        }

@asis
Copy link

asis commented Jun 5, 2017

What do you think about a simpler approach? In our case, it would be enough to document our components with a generic message whenever a spread is found.

So, for a component like this one:

MyComponent.propTypes = {
  /**
    * Component Foo proptypes
    */
  ...Foo.propTypes,
  /**
    * Some other proptypes
    */
  ...propTypes,
  /**
    * Unhandled spread
    */
  ...{
    bar: Prop.bool,
  }
};

We would like to get the following descriptors:

{
  "...Foo.propTypes": {
    "description": "Component Foo proptypes"
  },
  "...propTypes": {
    "description": "Some other proptypes"
  }
}

Right now, these proptypes are just ignored, resulting in incomplete docs :(

We have got a proof of concept fork which adds support for Identifier or MemberExpression spreads, ignoring other spread expressions.

I can create a PR if you think this approach is worth it :)

@fkling
Copy link
Member

fkling commented Jun 5, 2017

@asis: They are not ignored: https://github.com/reactjs/react-docgen/blob/00f2d2d62d93b9243157c4cea19cf241121a7721/src/handlers/__tests__/propTypeCompositionHandler-test.js, but they are stored separately from normal props.

@asis
Copy link

asis commented Jun 6, 2017

Oh, sorry! I was focused on extracting docs from the props declaration, so when I saw

it('understands and ignores the spread operator', () => {
I thought spread props were just being ignored altogether 😊

Thanks for the quick response!

@RIP21
Copy link

RIP21 commented Jun 6, 2017

+1
Have same use case as @asis We have our wrappers around semantic-ui components very often, and I try to spread their propTypes to have their props documented as well as ours but got nothing.
Just wrote some makes-no-sense component for test and parse it and got:

{
  "description": "Our wrapper component around semantic-ui button\r\nFor more props visit\r\n[semantic-ui-button](https://react.semantic-ui.com/elements/button#button-example-button)",
  "methods": [],
  "props": {
    "children": {
      "type": {
        "name": "any"
      },
      "required": false,
      "description": "Button label"
    }
  },
  "composes": [
    "semantic-ui-react"
  ]
}

Code:

import React from "react";
import PT from "prop-types";
import { Button as SButton } from "semantic-ui-react";

/**
 * Our wrapper component around semantic-ui button
 * For more props visit
 * [semantic-ui-button](https://react.semantic-ui.com/elements/button#button-example-button)
 */
const Button = ({ children, ...rest }) =>
  <SButton {...rest}>{children}</SButton>;

Button.propTypes = {
  /** Button label */
  children: PT.any,
  ...SButton.propTypes
};

export default Button;

Will be awesome to have this with comments that propTypes of the imported component have. Since all semantic and our components are react-docgen documented.

@fkling
Copy link
Member

fkling commented Jun 6, 2017

@RIP21
The recommended approach (which is also what we do at Facebook), is to merge the docs yourself. I.e. since you know that this component composes semantic-ui-react, you can look up the documentation for the imported component.

I guess in your specific case we would also have to record which export you are importing (Button).

@reintroducing
Copy link

I'm running into the same thing while using either propTypes = CustomPropTypes; or trying to destructure as propTypes = {...CustomPropTypes};. Is there any work around currently or is this still being discussed? I apologize but reading through the comments here it was not clear to me what the proposed solution is.

@asis
Copy link

asis commented Jul 26, 2017

We just used the list of "composed props" to infer the source files where they were defined. To solve the problem in styleguidist/react-styleguidist#548, we use a custom propsParser which turns each composed props reference into a "custom" prop, with a description containg a link to the relevant section of our docs. It is brittle... but it works.

@pasupuletics
Copy link

I have over come this limitation by writing my own custom handler, And would like to thank @benjamn for providing such a wonderful library called recast, which makes my life easy while dealing with AST. And big thanks to react-docgen too.

@reintroducing
Copy link

Would you mind sharing your handler?

@benwiley4000
Copy link

@pasupuletics sorry I corrected the links now.

@benjroy
Copy link

benjroy commented Dec 5, 2018

@pasupuletics and @siddharthkp got me inspired with their implementations. I forked their repo and expanded the functionality, wrote a whole lot of snapshot tests, and in the end mostly tweaked some code related to resolveToValue.

I'd like to offer this up in hopes that it will help others: https://github.com/benjroy/react-docgen-imported-proptype-handler

npm install react-docgen-imported-proptype-handler --save-dev

Setup is pretty much the same as react-docgen-external-proptypes-handler

The test fixtures exercise the expanded capabilities: https://github.com/benjroy/react-docgen-imported-proptype-handler/tree/master/src/handlers/__tests__/fixtures/components

And there is nothing computed in the final snapshot: https://github.com/benjroy/react-docgen-imported-proptype-handler/blob/master/src/handlers/__tests__/__snapshots__/importedPropTypeHandler-test.js.snap

it works well following imports of files in your repo.

following imports from node_modules: there is some basic path resolution and an implementation exercising it to make sure it won't throw, but I didn't write any logic to be able to read babel processed files from external dependencies

@benwiley4000
Copy link

@benjroy thank you for sharing this! I have tried setting it up in my project and I ran into some issues that are keeping me from building my docs with this setup.

Do you have an example of how you've integrated the package in your own project?

@benjroy
Copy link

benjroy commented Dec 5, 2018

@benwiley4000 thank you for trying it out! I forgot to ship @babel/runtime. Just published v1.0.1 for the fix.

Additionally, here is a repo with basic example usage:
https://github.com/benjroy/react-docgen-imported-proptype-handler-example
And the script itself:
https://github.com/benjroy/react-docgen-imported-proptype-handler-example/blob/master/scripts/docgen.js

@siddharthkp
Copy link

@benjroy That's brilliant!

If it works nicely, I don't mind deprecating my package. I'm not actively maintaining it anyways.

You can add @pasupuletics and me as maintainers if you like.

@reintroducing
Copy link

@benjroy Thanks for doing this. I'm unclear what the differences between your solution and this one is, however. Also, I think it would be good to show how this is used in the confines of react-styleguidist which is I think where all this originated, if possible. I'm also not able to see clearly how to use it there from your example.

@nemoDreamer
Copy link

@pasupuletics @siddharthkp and @benjroy , thanks for the great work!

I'm trying react-docgen-imported-proptype-handler as it has the most robust set of supported cases and fixtures, but I'm getting the following when doing this:

import BaseComponent from './BaseComponent';

// ...

MyComponent.propTypes = {
  ...BaseComponent.propTypes,
  // ... more
}

MyComponent.defaultProps = {
  ...BaseComponent.defaultProps,
  // ... more
}

export default MyComponent;

Error:

Cannot find module 'react-docgen/dist/babylon' from 'resolveImportedNamespace.js'
  at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:221:17)
  at Object.<anonymous> (node_modules/react-docgen-imported-proptype-handler/dist/utils/resolveImportedNamespace.js:24:39)

Since there is not error without your handler, and all the Babel stuff works nicely, I'm a bit stumped.
react-docgen and react-docgen-imported-proptype-handler both use [email protected], but I've got a different react-docgen version floating around from babel-plugin-react-docgen:

$ yarn list --pattern react-docgen

yarn list v1.13.0
├─ [email protected]
│  └─ [email protected]
├─ [email protected]
├─ [email protected]
└─ [email protected]

@nemoDreamer
Copy link

It seems that these issues could be side-stepped if this handler were simply a pull-request agains the official react-docgen repo, no? You're already copy/pasting a bunch of utils/ over, and the demand for this handler is so great that I'd be surprized if it didn't get accepted, right @fkling ?

@benjroy
Copy link

benjroy commented Feb 5, 2019

@nemoDreamer yea, I think this handler I wrote would be better implemented as a PR into react-docgen (the meat of the changes is slight tweaks to some of the utility methods to pass a filepath around).

Your problem specifically is that react-docgen v3 doesn't ship the babylon parser.

I noticed too late when i was working on the handler that react-docgen was in alpha for major version 3.0.0, but tl;dr;:
this handler was only tested with react-docgen ^2.x.x

  • you will get bit by the babel-plugin-react-docgen shipping docgen v3
  • you will probably get bit by recast (this bit me because there were two competing versions of recast in my node deps tree. In order for the handler to work, docgen and this handler MUST be pointing to the exact same recast dependency in node-modules

Reworking this for react-docgen v3.0.0 will probably require a bit more work because of the dropped babylon dep, and at that point the effort would be much better spent working this directly into react-docgen itself

@albertoblaz
Copy link

Haven't tried your package yet @benjroy but if it fixes the problem, it'd be awesome to see it merged into react-docgen because a lot of people seem to have the same issue and it's been 3 years since it was created

@luknoj
Copy link

luknoj commented Feb 12, 2019

Can this also solve the same problem with imported flow types?

@nicholaai
Copy link

any progress here?

@devongovett
Copy link
Contributor

I started hacking on this today and managed to get importing working for propTypes, flow, and typescript types. See devongovett/react-docgen@typescript...devongovett:importing. I'll open up a PR once the typescript PR (#348) is merged, since this builds on that. In the meantime, feel free to try out my branch.

Current limitations:

  • ES6 imports only. Could add CommonJS support, but it's a bit more complicated and I'm not sure it's worth it when most apps should be using ES6 now anyway.
  • Namespace imports are unsupported (import * as foo from '...')
  • Wildcard re-exports are unsupported (export * from '...')
  • Resolving from node_modules is supported, but flow libdefs (flow-typed folder, .js.flow files, and .d.ts files) are unsupported. Would need to reimplement a custom resolver for those.

Please let me know if you try it out and encounter bugs.

@devongovett
Copy link
Contributor

PR opened: #352

@Hellycat
Copy link

There is an issue with PropTypes.oneOf(['a', 'b']). It creates next error: TypeError: Cannot read property 'type' of null

@saviomd
Copy link

saviomd commented Sep 15, 2022

I have the same problem, is this being worked on?
@Tauana-Pacheco FYI

@brianespinosa
Copy link

Dropping a link to this conversation to help my future self when I inevitably end up here in a couple months.

@RIP21
Copy link

RIP21 commented Sep 17, 2022

Dropping a link to this conversation to help my future self when I inevitably end up here in a couple months.

Just my 5 cents. Anyone who is still using prop-types should really invest in Typescript instead. Prop-types are quite dead at this point. While, I'm sure, many are still using prop-types, it is extremely dated approach and causes more issues than solves IMO. There are plenty of codemodes that can help you with migration.

@brianespinosa
Copy link

@RIP21 the linked conversation is relevant for Typescript types...

@danez danez added this to the v6 milestone Sep 27, 2022
@danez danez closed this as completed Jun 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.