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

Replace doctrine with another JSDoc parser #1708

Open
jasonwilliams opened this issue Oct 31, 2020 · 12 comments
Open

Replace doctrine with another JSDoc parser #1708

jasonwilliams opened this issue Oct 31, 2020 · 12 comments

Comments

@jasonwilliams
Copy link
Collaborator

jasonwilliams commented Oct 31, 2020

As part of the Webpack 5 investigation it was discovered doctrine uses some node modules which WP5 no longer polyfills by default. Doctrine itself is end of life and no longer supported. It would be good for styleguidist to move off it permanently and replace it with another solution.

Doctrine is used in these places:
https://github.com/styleguidist/react-styleguidist/search?q=doctrine

@poteirard
Copy link
Contributor

I guess jsdoctypeparser would be a good alternative? or maybe https://swc.rs/rustdoc/jsdoc/ would be good for performance?

@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Nov 2, 2020

@poteirard it could be yeah. Best way is to look at what doctrine is doing and seeing if the same can be done with another parser. The swc one looks like a rust crate, is that on npm?

@sapegin
Copy link
Member

sapegin commented Nov 3, 2020

Looks like it's also used by react-docgen so replacing it in Styleguidist won't solve the problems with webpack 5? It's still a good idea to replace it though.

Looks like jsdoctypeparser does only half of the job: it parses the types (String[]) but we also need to find JSDoc declarations inside comments in the source code.

sapegin pushed a commit that referenced this issue Nov 5, 2020
Refs #1703

* Inline loaders and ! prefixes should not be used as they are non-standard. They may be used by loader generated code.
https://webpack.js.org/configuration/module/#ruleenforce
* Adds support for both Webpack 4 and 5 in StyleguidistOptionsPlugin
* Updates dependencies in Webpack example (easier to test against)
* Because automatic polyfilling is switched off in 5, assert was brought in as a dependency (it's used by Doctrine, see below)

## Upstream issues

Both issues below are tied to facebook/create-react-app#7929:

* Process is not defined - The page still builds but this error will show in the console. I can't seem to polyfill this if anyone else can that would be great, then I think we're done.
* TypeError: message.split is not a function - you may not get this error, but if you do it's related to facebook/create-react-app#7929. The quick fix is to go into node_modules/react-dev-utils/formatWebpackMessages.js:19 and add the code from facebook/create-react-app#7929 (comment)

## Not needed for this but nice to have.

Doctrine should be replaced with another JSDoc parser. Doctrine is end of life and no longer supported. It causes problems with Webpack 5 because it pulls in assert which WP5 does not polyfill. For now, we can fix it by adding those polyfills (assert) but a more stable solution should be found. The issue raised: #1708
@poteirard
Copy link
Contributor

I found https://github.com/syavorsky/comment-parser does the other half of the job. I'm trying to make it work but since it's not working exactly the same as the other I'm struggling a bit to make it work (WIP work: 7df84e8)

@sapegin
Copy link
Member

sapegin commented Nov 6, 2020

This library looks good! I think a better solution would be to extract all the code that parses JSDoc into its own module that would return normalized object with merge synonyms and so on, instead of using these libraries directly in getProps.

@poteirard
Copy link
Contributor

poteirard commented Nov 6, 2020

Sorry, not sure if I understood correctly. You mean to modify react-docsgen with these libraries and then use it in react-styleguidist?

@sapegin
Copy link
Member

sapegin commented Nov 6, 2020

I mean to make a new module inside Styleguidist that would generate usable object with JSDoc data, something like:

function getJsDocTags(code) {
  // magic
  return {
    params: [...],
    returns: 'string',
    // ...
  }
}

The current approach is too verbose and brittle, and it'll be worse with two libraries.

@poteirard
Copy link
Contributor

So you mean in the props-loder.ts I need to replace getProps by a new module which has a getJsDocTags that generates the same output, right?

@sapegin
Copy link
Member

sapegin commented Nov 7, 2020

Not exactly ;-) we'll keep getProps but most lines you changed in your branch would go to this new module. So I want to encapsulate:

  • Parsing JSDoc from the source code (or whatever we do here).
  • Parsing each JSDoc declaration.
  • Merging aliases, like params and returns.
  • Building a typed (meaning we have types for each specific tag not generic types) object that we could use later in getProps.

@poteirard
Copy link
Contributor

Trying again :)
Something I noticed is the optional parameters are different now and all the type keys.
But:

  1. I'm not sure how to fix the merge with the description if someone can give me a hand it will be nice. It's hard for me to follow.
  2. The react-docgen types are incompatible with the new implementation. So I'm not sure if <ArgumentRenderer/> (line 43) will work as expected if I use the stringifying from the new library . Would it make sense to upgrade react-docgen first given doctrine is no longer maintained?

https://github.com/poteirard/react-styleguidist/tree/replace_doctrine

@sapegin
Copy link
Member

sapegin commented Nov 10, 2020

Updating react-docgen is always a good idea!

Could you open a pull request from your branch? It would be much easier to check.

@poteirard
Copy link
Contributor

Created: #1711

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

Successfully merging a pull request may close this issue.

3 participants