-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 storyshots with new babel config #1721
Conversation
//cc @ndelangen @shilman It turns out there was one more issue regarding storyshots on the newest releases. |
Codecov Report
@@ Coverage Diff @@
## master #1721 +/- ##
=======================================
Coverage 21.18% 21.18%
=======================================
Files 252 252
Lines 5694 5694
Branches 684 692 +8
=======================================
Hits 1206 1206
+ Misses 3965 3941 -24
- Partials 523 547 +24
Continue to review full report at Codecov.
|
app/react/src/server/config/babel.js
Outdated
@@ -8,7 +8,7 @@ module.exports = { | |||
targets: { | |||
browsers: ['last 2 versions', 'safari >= 7'], | |||
}, | |||
modules: false, | |||
modules: process.env.NODE_ENV === 'test' ? undefined : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we should do undefined
here or commonjs
. the default for this is commonjs
but I'm worried it might change in future versions and break support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it. Anyways, if they change the default it will be a major bump.
Want me to set to commonjs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah let's do commonjs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! Also did the same for vue and react-native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on crna-kitchen-sink
and cra-kitchen-sink
We don't have storyshots set up for vue-kitchen-sink
yet.
Issue: #1720
What I did
Set
modules
to'commonjs'
when NODE_ENV === 'test'.How to test
Simply test against the storyshot demos.