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

Move react-docgen dependency to docs addon preset. #9533

Closed

Conversation

patricklafrance
Copy link
Member

@patricklafrance patricklafrance commented Jan 18, 2020

Issue: #9273

What I did

  • Moved babel-plugin-react-docgen and babel-plugin-add-react-displayname from app/react to docs addon.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Jan 18, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/rgd37jn4w
✅ Preview: https://monorepo-git-move-react-docgen-dependency-to-docs-preset.storybook.now.sh

@patricklafrance
Copy link
Member Author

patricklafrance commented Jan 18, 2020

Hey @shilman any idea why it doesn't work?

I get the following for every components, PropType or TS:

image

I am using cra-ts-kitchen-sink to test.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 18, 2020

Fails
🚫 PR is marked with "do not merge" label.

Generated by 🚫 dangerJS against 8e7e05d

@patricklafrance
Copy link
Member Author

@shilman nvm

@patricklafrance
Copy link
Member Author

Still failing with the following error, not sure what to do here:

image

@shilman
Copy link
Member

shilman commented Jan 21, 2020

@patricklafrance how do i reproduce this problem? i updated from next and am not seeing it

@shilman shilman added this to the 6.0.0 milestone Jan 21, 2020
@patricklafrance
Copy link
Member Author

@shilman it's working now, seems like the problem might have been fixed in next?

@patricklafrance
Copy link
Member Author

patricklafrance commented Jan 21, 2020

@shilman As part of this PR I am also moving babel-plugin-add-react-displayname to the docs preset. Is it a good idea? Do you know if it's used by another feature of SB?

@shilman
Copy link
Member

shilman commented Jan 21, 2020

Hmm I think both are used by addon-info. I'm thinking we should deprecate addon-info in 6.0, but we still have to have a solution for it.

@patricklafrance
Copy link
Member Author

patricklafrance commented Jan 21, 2020

@shilman ok I thought deprecating addon-info was a done deal in 6.0, that's why I started this PR.

What does addon-info do that docs doesn't?

@shilman
Copy link
Member

shilman commented Jan 21, 2020

@patricklafrance Deprecating does not mean deleting. It's still used 3x as much as addon-docs, so it means there are a bunch of people out there who rely on it. We need to formally announce that addon-info is deprecated THEN give people time to migrate off.

After a little thought, here's my addon-info proposal. I think we can use the same playbook for addon-notes, polymer, and any other package we want to EOL in 6.0 cc @tmeasday @ndelangen:

  • Move addon-info into its own repo
  • Move all existing addon-info issues to the new repo (if possible)
  • Add deprecation warnings with a directive to upgrade to addon-docs
  • Add deprecation warning to the top of the README
  • Add migration instructions for how to use main.js config to get babel-plugin-react-docgen and babel-plugin-add-react-displayname for addon-info
  • Immediately close all incoming addon-info issues and direct author to new repo

@patricklafrance
Copy link
Member Author

Ok I see @shilman i’ll close this PR then since we can’t move react-docgen!

@shilman
Copy link
Member

shilman commented Jan 21, 2020

@patricklafrance you missed my point. we can move it. we just need to document how to manually install the babel plugins for for addon-info users.

@shilman shilman reopened this Jan 21, 2020
@patricklafrance
Copy link
Member Author

@shilman ahh got it! :)

@ndelangen
Copy link
Member

I can put this on my todo-list:

Move addon-info into its own repo

@patricklafrance
Copy link
Member Author

Would be great @ndelangen, thank you!

@ndelangen
Copy link
Member

@tmeasday I'm not sure if I should be assigned to this.

I proposed I'd handle the extraction of addon-notes & addon-info from the monorepo.

This PR itself deals with a different issue (which I do not have a lot of context on)?

@ndelangen
Copy link
Member

@patricklafrance Is this PR completely blocked until addon-notes & addon-info are extracted?

@shilman shilman assigned shilman and unassigned ndelangen Jan 28, 2020
@shilman
Copy link
Member

shilman commented Jan 28, 2020

I'll take this one @ndelangen @tmeasday

But it won't happen until after I get back from vacation this weekend. Thanks for your patience @patricklafrance

@shilman shilman modified the milestones: 6.0.0, 6.0 breaking changes Mar 10, 2020
@shilman
Copy link
Member

shilman commented May 22, 2020

Won't fix. Took a different approach in #8444

Users still take the dependency hit, but it's an easy to change to decide which docgen you want

@shilman shilman closed this May 22, 2020
@stof stof deleted the move-react-docgen-dependency-to-docs-preset branch May 25, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants