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

Build fails with JSDocs tags in Component docs. (webpack 2, styleguide 5 beta) #298

Closed
AoDev opened this issue Jan 31, 2017 · 19 comments
Closed

Comments

@AoDev
Copy link
Contributor

AoDev commented Jan 31, 2017

Environment:

  • Webpack version: 2.2.0
  • react-styleguidist: 5.0.0-beta.10

The build breaks because of JSDocs tags.
We have updated to Webpack 2 and after checking this issue: #292 I have decided to go with the v5 beta.

I consistently manage to reproduce the build error with @see and @module JSDocs tags. I haven't tried other tags. We had an @see that I have temporarily removed from our code to get it to work. I don't know if it's style guide V5 specific.

Example:

Pagination.js

/**
 * Some random doc comments
 *
 * @see some-url
 */
export default function Pagination (props) {
  return <div/>
}

Error thrown:

Failed to compile.

Error in ./src/framework/components/Pagination/Pagination.js
Module build failed: TypeError: obj.hasOwnProperty is not a function
    at Array.map (native)
 @ ./~/react-styleguidist/loaders/styleguide-loader.js!./~/react-styleguidist/lib/index.js 128:29-265
 @ ./~/react-styleguidist/lib/index.js
 @ multi ./~/react-dev-utils/webpackHotDevClient.js ./src/framework/docs/index.js ./~/react-styleguidist/lib/index
@sapegin
Copy link
Member

sapegin commented Jan 31, 2017

Maybe not 5.x specific be definitely a bug, I‘ll try to debug and fix it (but feel free to submit a pull request if you have time and will to fix it).

@sapegin sapegin added the bug label Jan 31, 2017
@sapegin sapegin modified the milestone: 5.0.0 Jan 31, 2017
@sapegin
Copy link
Member

sapegin commented Feb 2, 2017

I found a reason of this error but not sure about the right way to fix that.

  1. react-docgen creates an object without prototype here: https://github.com/reactjs/react-docgen/blob/d65bfc460d7274206c3208fc41518727acd5a490/src/utils/docblock.js#L58

  2. Then to-ast tries to call hasOwnProperty on this object and fails: https://github.com/devongovett/to-ast/blob/master/index.js#L128

Of course we can find a workaround that (read “hack”) but probably one of these libraries should be fixed instead.

@sapegin
Copy link
Member

sapegin commented Feb 2, 2017

I’ve created an issue at react-docgen: reactjs/react-docgen#155

@AoDev
Copy link
Contributor Author

AoDev commented Feb 2, 2017

Thanks for investigating. I think it was right to ask why they do that. Would be nice that they fix it if there is no good reason.

@n1313
Copy link
Collaborator

n1313 commented Feb 13, 2017

@AoDev could you please take a look and check if my fix worked for you?

@AoDev
Copy link
Contributor Author

AoDev commented Feb 13, 2017

@n1313 Thanks! I'll check a bit later. On my side I made a micro PR in react-docgen.

@AoDev
Copy link
Contributor Author

AoDev commented Feb 14, 2017

@n1313 Is it possible to release a '5.0.0-beta.11' with the fix included? this way it would be easier to test. I have tried to install by using the next branch (git://github.com/styleguidist/react-styleguidist.git#next) but get an error. Maybe some other stuff related to the config have changed since then.

Error in multi ./~/react-dev-utils/webpackHotDevClient.js ./src/framework/docs/index.js ./~/react-styleguidist/lib/index
Module not found: Can't resolve '.../node_modules/react-styleguidist/lib/index' in '/Volumes/DataUser/git-repos/risingStack/trace/trace-client'

 @ multi ./~/react-dev-utils/webpackHotDevClient.js ./src/framework/docs/index.js ./~/react-styleguidist/lib/index

@sapegin
Copy link
Member

sapegin commented Feb 14, 2017

@AoDev It must be because Babel compilation is needed. I’ll try to make a new beta tomorrow.

@AoDev
Copy link
Contributor Author

AoDev commented Feb 14, 2017

@sapegin How would be the easiest way for me to pull the project and try it locally? if I were to clone the repo in the node-modules of my project and generate the dist, would it work?

@sapegin
Copy link
Member

sapegin commented Feb 14, 2017

@AoDev Yeah, that would be the easiest way. Clone and then do npm i && npm run compile.

@AoDev
Copy link
Contributor Author

AoDev commented Feb 14, 2017

@sapegin @n1313 I just tried by pulling the repo and compiling like sapegin said. It compiled and your fix works. At least for the small @see I had.

I don't know what is the expected output but currently it is stripped out from the documentation. I'd rather have them in, unless you plan to render jsDocs tags in some way later.

Thank you both and I'll wait for the new beta :)

@sapegin
Copy link
Member

sapegin commented Feb 14, 2017

Maybe we should add a config option to define renderers for unknown (to Styleguidist) tags. Not sure it‘s a good idea to render everything ;-) But it‘s better to open another issue to discuss it.

@n1313
Copy link
Collaborator

n1313 commented Feb 15, 2017

I was thinking of adding some support for @see tag to RSG, it could be useful for my project. @AoDev, could you please give me an example of how you are currently using this tag in your project? What values does it take and what purpose does it serve?

@AoDev
Copy link
Contributor Author

AoDev commented Feb 15, 2017

I have three kinds of value:

  • url / link to some external website.
    @see http://some-website...

  • name of a function in the same module. (rare in the case of UI components)
    @see getLabel

  • name of another component in the framework.

/**
 * Has additional behaviours...
 **/
function InputBase {
  return <input...
}
/**
 * Inherit behaviour of InputBase and adds default styles
 * @see InputBase
 **/
function Input {
  return <InputBase...
}

@sapegin
Copy link
Member

sapegin commented Feb 15, 2017

@n1313 @AoDev Guys could we please close this issue and discuss a new feature in a new issue? 🌚

@n1313
Copy link
Collaborator

n1313 commented Feb 15, 2017

@AoDev Thanks! I'm closing this issue and will ping you when I'm ready to start on adding support for @see.

@n1313 n1313 closed this as completed Feb 15, 2017
@AoDev
Copy link
Contributor Author

AoDev commented Feb 15, 2017

Not sure I would close the issue. Sure, the fix seems to work but it's not available to people yet. I'd rather close an issue when it's available. I have not tried in v4. Only v5 beta so I don't know if v4 was affected.

@sapegin
Copy link
Member

sapegin commented Feb 15, 2017

It’d 5.x only issue, we don’t use to-ast in 4.x.

@AoDev
Copy link
Contributor Author

AoDev commented Feb 15, 2017

Cool, thanks:)

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

3 participants