-
-
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
Remove the cacheDirectory
option from babel config
#1350
Conversation
In storyshots, we are running babel directly, not through webpack, so we can't use this option, which applies to `babel-loader`. See https://github.com/storybooks/storybook/blob/master/app/react/src/server/config/babel.js#L15 #1254
addons/storyshots/src/index.js
Outdated
const configDirPath = path.resolve(options.configPath || '.storybook'); | ||
configPath = path.join(configDirPath, 'config.js'); | ||
|
||
const babelConfig = loadBabelConfig(configDirPath); | ||
// We set this in the default babel config, but it is webpack-only | ||
delete babelConfig.cacheDirectory; |
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.
Potentially we should remove this config from this file: https://github.com/storybooks/storybook/blob/master/app/react/src/server/config/babel.js#L15= and instead add it when adding the config to webpack.
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.
Sounds better!
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.
OK, done! Please take a look at the last commit.
Along the way this actually improves the behaviour in RN, before it wasn't using a cache dir if you had a custom .babelrc
(which I reckon you always would in RN)
This issue would only surface if you ran |
Codecov Report
@@ Coverage Diff @@
## master #1350 +/- ##
==========================================
- Coverage 13.75% 13.73% -0.03%
==========================================
Files 202 202
Lines 4623 4623
Branches 616 598 -18
==========================================
- Hits 636 635 -1
- Misses 3453 3473 +20
+ Partials 534 515 -19
Continue to review full report at Codecov.
|
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.
@tmeasday @ndelangen I don't actually understand this completely, so I'm going to approve because it makes sense, but I'll let @ndelangen approve/merge.
@ndelangen merging this with @tmeasday 's updates, which have been tested for both react and RN. going to do a small release to get this and other patch fixes out. |
Issue: #1254
What I did
In storyshots, we are running babel directly, not through webpack, so we can't use this option, which applies to
babel-loader
. See https://github.com/storybooks/storybook/blob/master/app/react/src/server/config/babel.js#L15=How to test
Trusty ol'
test-cra
, runningnpm test
from insideexamples/test-cra
.