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.

Sign up for GitHub

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

Jump to bottom

chore(rollup): assign babel dev env when running rollup #229

Merged
merged 1 commit into from
Dec 28, 2018
Merged

chore(rollup): assign babel dev env when running rollup #229

merged 1 commit into from
Dec 28, 2018

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Dec 28, 2018

Fixes #228

This is a simple proposal to fix the prop-types removal when running rollup builds.

@montezume
Copy link
Contributor

Can you check what other libraries do? Since we don't run babel on this code from the MC-FE, wouldn't the prop types be left then in production?

@emmenko
Copy link
Member Author

emmenko commented Dec 28, 2018

I looked into some libraries yes but couldn't find anything useful. I'll keep looking.

Do you have other ideas how we can solve this?

@emmenko
Copy link
Member Author

emmenko commented Dec 28, 2018

Might be related: facebook/create-react-app#209

What we could do maybe is to use the option mode: wrap: https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types#mode

This would ship the bundle with the process.env.NODE_ENV !== "production" condition, which then webpack should pick up when building the production application bundle.

What do you think?

@montezume
Copy link
Contributor

@emmenko that's exactly what I see in the downshift library. If it's good enough for KCD 🤔

@emmenko
Copy link
Member Author

emmenko commented Dec 28, 2018

Alright, this should work now.

Ref (https://github.com/kentcdodds/kcd-scripts/blob/master/src/config/babelrc.js#L70-L71)

montezume
montezume suggested changes Dec 28, 2018
View reviewed changes
Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

I don't think this will work for us, and it doesn't seem to support custom proptypes.

@montezume
Copy link
Contributor

Seems to be an open issue with unsafe-wrap.
oliviertassinari/babel-plugin-transform-react-remove-prop-types#175
I tried "wrap" on ui-kit and it built with no errors.

montezume
montezume approved these changes Dec 28, 2018
View reviewed changes
Copy link
Contributor

@montezume montezume left a comment

Choose a reason for hiding this comment

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

Thanks. I guess we can keep track of the issue going forward

tdeekens
tdeekens approved these changes Dec 28, 2018
View reviewed changes
packages/babel-preset-mc-app/index.js
@@ -10,6 +10,7 @@ module.exports = function getBabePresetConfigForMcApp() {
const isEnvDevelopment = env === 'development';
const isEnvProduction = env === 'production';
const isEnvTest = env === 'test';
const isRollup = process.env.BUILD_ROLLUP === true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Build tool sniffing feels a bit odd but I can not think of anything else other than solving the issue somewhere entirely different.

@emmenko emmenko merged commit 70db16b into master Dec 28, 2018
@emmenko emmenko deleted the nm-rollup-babel-env-dev branch December 28, 2018 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdeekens tdeekens tdeekens approved these changes

@montezume montezume montezume approved these changes

Assignees
No one assigned
Labels
🔮 Type: Chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Babel and rollup remove prop-types in production builds
3 participants
@emmenko @montezume @tdeekens