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

Support for react-docgen@5.0.0 #5

Merged
merged 1 commit into from
Jul 2, 2019

Conversation

smashercosmo
Copy link
Contributor

This PR adds support for react-docgen@5.0.0. In addition I've also made some cleanup, like:

  1. Bumped dependencies
  2. Removed babel transformation (because react-docgen doesn't officially support browsers, so this module also doesn't have to)
  3. Added proper settings for ESLint and Prettier
  4. Removed unneeded files

If it's too much for one PR, I can close this one and submit another one, that will contain changes only related to react-docgen@5.0.0 support

P.S. One thing I'm not sure about is what type of dependencies ast-types and react-docgen should be. Dependencies or peerDependencies? For now I left them in devDependencies.

@sapegin
Copy link

sapegin commented Jun 6, 2019

I guess react-docgen should be a peerDependency if we expect a specific version of it.

@sapegin
Copy link

sapegin commented Jun 6, 2019

@Jmeyering any chance you can have a look at this PR? 👋

@smashercosmo
Copy link
Contributor Author

smashercosmo commented Jun 6, 2019

@sapegin what about ast-types package? What type of dependency is it? I guess it should always be the same version react-docgen uses.

@sapegin
Copy link

sapegin commented Jun 6, 2019

Can it be a normal dependency? It has no dependencies itself. The worst thing that can happen, it won't be deduped and we'll ship two version to the client.

"*.{js,jsx,json}": [
"prettier --trailing-comma all --write",
"*.js": [
"npm run prettier",
Copy link

Choose a reason for hiding this comment

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

just run prettier --write here instead. Otherwise the *.js from the script going to render all benefits of lint-staged to zero

Copy link

@okonet okonet Jun 6, 2019

Choose a reason for hiding this comment

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

I'd also suggest to run eslint --fix here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. thx.

"prettier"
],
"rules": {
"prettier/prettier": [
Copy link

Choose a reason for hiding this comment

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

Since you use this plugin, you might not need running prettier --write in lint-staged at all. just eslint --fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. thx.

Copy link

Choose a reason for hiding this comment

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

I'd advice against that plugin: it's very annoying when you develop, because half of your code is marked as error, but it's just a formatting "issue" that can be autofixed. But you don't want to be distracted when you write code.

@Jmeyering Jmeyering merged commit 049b470 into Jmeyering:master Jul 2, 2019
@Jmeyering
Copy link
Owner

Merged and published under 2.0.0

Sorry for the delayed response, missed the initial merge request email apparently.

@smashercosmo
Copy link
Contributor Author

Hey @Jmeyering thx for merging. Do you want me to provide changes advised by @sapegin and @okonet in separate PR?

@okonet
Copy link

okonet commented Jul 2, 2019

@Jmeyering we’re in the process of switching to monorepo for styleguidist. Would you mind having this package there as a separate package?

@Jmeyering
Copy link
Owner

Jmeyering commented Jul 2, 2019 via email

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

Successfully merging this pull request may close these issues.

None yet

4 participants