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 storyshots by moving cacheDirectory to webpack config #1713

Merged
merged 3 commits into from
Aug 23, 2017

Conversation

ndelangen
Copy link
Member

fixes: #1709

What I did

Moved cacheDirectory back to webpack config from babel config

How to test

See issue (I think it should be apparent when using storyshots?)

@ndelangen ndelangen added maintenance User-facing maintenance tasks configuration babel / webpack labels Aug 22, 2017
@ndelangen ndelangen self-assigned this Aug 22, 2017
@ndelangen ndelangen requested a review from a team August 22, 2017 22:05
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.

LGTM

@tmeasday
Copy link
Member

Hmm, I felt like I had done this already ;) #1350

@tmeasday
Copy link
Member

RE: testing -- yes this was a storyshots issue, test the various app modes in SS.

@ndelangen ndelangen force-pushed the fix/1709-cacheDirectory branch from 887c572 to 2890dd3 Compare August 22, 2017 22:52
@codecov
Copy link

codecov bot commented Aug 22, 2017

Codecov Report

Merging #1713 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1713      +/-   ##
==========================================
- Coverage   21.24%   21.23%   -0.02%     
==========================================
  Files         252      252              
  Lines        5700     5698       -2     
  Branches      685      682       -3     
==========================================
- Hits         1211     1210       -1     
- Misses       3947     3975      +28     
+ Partials      542      513      -29
Impacted Files Coverage Δ
app/vue/src/server/config/babel.js 100% <ø> (ø) ⬆️
app/react/src/server/config/babel.js 0% <ø> (ø) ⬆️
app/vue/src/server/config.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 41.17% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 48.8% <0%> (ø) ⬆️
addons/knobs/src/components/types/Select.js 7.81% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
... and 16 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 c291483...d0b5176. Read the comment docs.

@@ -3,7 +3,6 @@ const findCacheDir = require('find-cache-dir');
module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused findCacheDir above.

@@ -11,10 +12,16 @@ const logger = console;
// (inside working directory) if a config path is not provided.
export default function(configType, baseConfig, configDir) {
const config = baseConfig;

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra space.

const babelConfig = loadBabelConfig(configDir);
config.module.rules[0].query = babelConfig;

config.module.rules[0].query = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be applied to the react app too?

@Hypnosphi
Copy link
Member

Seems like we .eslintignore everything related to vue: https://github.com/storybooks/storybook/blob/master/.eslintignore#L9
Is there any good reason for that?

@Hypnosphi
Copy link
Member

That's what I get when I remove this line:

111 problems (10 errors, 101 warnings)
0 errors, 85 warnings potentially fixable with the `--fix` option.

Seems doable, I'll try to handle it

@Hypnosphi
Copy link
Member

Here it is #1715

@ndelangen ndelangen merged commit 5ee5038 into master Aug 23, 2017
@ndelangen ndelangen deleted the fix/1709-cacheDirectory branch August 23, 2017 06:00
@satazor
Copy link
Contributor

satazor commented Aug 23, 2017

❤️

Could we have a patch release? :D

@shilman shilman changed the title FIX #1709 Fix storyshots by moving cacheDirectory to webpack config Aug 23, 2017
@shilman shilman added the bug label Aug 23, 2017
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.

5 participants