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

Fix Docgen in static builds for Info #1725

Merged
merged 8 commits into from
Aug 28, 2017
Merged

Conversation

danielduan
Copy link
Member

@danielduan danielduan commented Aug 23, 2017

Issue:
Fixes: #1661 #1141

PropTypes get removed during our build and Docgen never runs so the Prop Table in Info shows up empty and undefined.

What I did

  • set webpack static build environment to dev to prevent PropTypes from being stripped out
  • added docgen plugin to static build babelrc
  • created new story component in example to show PropType table from fallback method
  • updated storyshots

How to test

run cra-kitchen-sink and check the 3 AddonInfo buttons.

Is this testable with jest or storyshots?
yes
Does this need a new example in the kitchen sink apps?
yes
Does this need an update to the documentation?
no
If your answer is yes to any of these, please make sure to include it in your PR.

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 23, 2017

set webpack static build environment to dev to prevent PropTypes from being stripped out

Can we find a way to restrict this to prop-types package? And maybe make this optional, as it's only needed for info plugin

@danielduan
Copy link
Member Author

danielduan commented Aug 24, 2017

@Hypnosphi I think the babel-preset-react is the one that strips out the PropTypes when compiling for production. I'll see what I can do there in terms of making that run in dev. We use this as a fallback for babel-plugin-react-docgen because docgen only works when PropTypes are defined in the same file as the component. Alternatively, we can just remove this fallback.

We don't really have a framework for turning features on and off in Webpack without using a custom Webpack config. Sadly, we never enabled custom .babelrc configs for static builds.

New issue created to track custom babel config: #1731

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 24, 2017

I think the babel-preset-react is the one that strips out the PropTypes when compiling for production

As far as I know, this logic lives inside prop-types package

UPD: yeah, here it is: https://github.com/facebook/prop-types/blob/master/index.js#L10

@danielduan
Copy link
Member Author

danielduan commented Aug 24, 2017

Actually yeah, I was thinking of how flow types are stripped out. I can't find any documentation on how to supply the NODE_ENV other than the link on the bottom of the readme. https://github.com/facebook/prop-types#proptypescheckproptypes which links to https://facebook.github.io/react/docs/optimizing-performance.html#webpack

That's what I'm doing already to unset the production flag. Webpack just feels like a black box to me. Any other suggestions?

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 24, 2017

One possible solution is to add custom loader just for prop-types package which will do nothing but replace ‘process.env.NODE_ENV‘ with ‘“development“‘

@ndelangen ndelangen changed the base branch from master to release/3.3 August 25, 2017 21:31
@danielduan
Copy link
Member Author

@Hypnosphi That feels really hacky.

I don't think the benefits of avoiding a dev webpack flag outweigh the cost of maintaining a custom webpack plugin.

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 27, 2017

That feels really hacky

That's exactly you now do for all the packages (via DefinePlugin).

maintaining a custom webpack plugin.

It should be rather a loader than plugin, and it would be a one-liner:

module.exports = source => source.replace(/process\.env\.NODE_ENV/g, '"development"');

I believe that using more performant versions of packages (as mush as we can) in what's supposed to be a production build outweighs maintaining one line of javascript a bit

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Please limit the scope of dev flag to prop-types package (see above) or remove it completely.

Please make al those changes optional (as you don't need it if you're not using info plugin)

@danielduan danielduan changed the title Fix PropType and Docgen in static builds for Info Fix Docgen in static builds for Info Aug 27, 2017
@danielduan
Copy link
Member Author

danielduan commented Aug 27, 2017

Removed the webpack dev environment. Definitely agree on the performance but I think making webpack settings optional is out of scope of this bug fix. Probably something like #1098

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 27, 2017

Well, now it's only babel, but maybe it won't hurt to have this plugin always

Please address the unit tests failure

@danielduan
Copy link
Member Author

how can I get the file-stub locally without modifying it manually?

    -      src="logo.svg"
    +      src="file-stub"

@codecov
Copy link

codecov bot commented Aug 27, 2017

Codecov Report

Merging #1725 into release/3.3 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/3.3    #1725   +/-   ##
============================================
  Coverage        23.12%   23.12%           
============================================
  Files              253      253           
  Lines             5756     5756           
  Branches           694      689    -5     
============================================
  Hits              1331     1331           
- Misses            3906     3934   +28     
+ Partials           519      491   -28
Impacted Files Coverage Δ
app/react/src/server/config/babel.prod.js 0% <ø> (ø) ⬆️
addons/info/src/components/PropTable.js 21% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.86% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_routing.js 28.04% <0%> (ø) ⬆️
...s/left_panel/stories_tree/tree_decorators_utils.js 45.23% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
addons/info/src/components/markdown/htags.js 30% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 30.98% <0%> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1e6ab7...0eb71b9. Read the comment docs.

@danielduan
Copy link
Member Author

@ndelangen @shilman any thoughts on this getting merged into master instead of 3.3? it's a small change to prod babel config and a new example app story.

@danielduan danielduan changed the base branch from release/3.3 to master August 28, 2017 13:48
@danielduan danielduan changed the base branch from master to release/3.3 August 28, 2017 15:21
@danielduan
Copy link
Member Author

danielduan commented Aug 28, 2017

Gonna just merge it into 3.3.

@danielduan danielduan merged commit 6f5b71c into release/3.3 Aug 28, 2017
@danielduan danielduan deleted the dd/docgen-static branch August 28, 2017 15:42
@shilman shilman mentioned this pull request Aug 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants