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

Exit storybook build non-zero on stats errors (e.g. errors in the transpilation pipeline) #1372

Merged
merged 4 commits into from
Jun 28, 2017

Conversation

joscha
Copy link
Member

@joscha joscha commented Jun 28, 2017

Currently, storybook does not exit non-zero when there are errors in the pipeline, e.g. we are using TypeScript with ts-loader and a type error that fails tsc does not make storybook exit non-zero even though the transpilation failed.

Issue:

What I did

Run build-storybook with a "broken" typescript source.

How to test

Any transpiler (for example typescript) producing an error will still not make storybook exit non-zero.

Currently, storybook does not exit non-zero when there are errors in the pipeline, e.g. we are using TypeScript with `ts-loader` and a type error that fails tsc does not make storybook exit non-zero even though the transpilation failed.
return logger.error(e);
});
}
if (err || stats.hasErrors()) {
Copy link
Member

Choose a reason for hiding this comment

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

if(err || stats.hasErrors()) {
  const errors = err ? [err] : stats.toJson().errors
  errors.forEach(...)
  process.exit(1)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

that would not print the stats errors if there is a "normal" error - is that deliberate?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, main point is that i think it should just be a single if statement

Copy link
Member Author

Choose a reason for hiding this comment

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

got it, changes in 9c57ae0

@codecov
Copy link

codecov bot commented Jun 28, 2017

Codecov Report

Merging #1372 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1372      +/-   ##
==========================================
- Coverage    14.1%   14.09%   -0.01%     
==========================================
  Files         201      201              
  Lines        4609     4611       +2     
  Branches      502      583      +81     
==========================================
  Hits          650      650              
+ Misses       3521     3463      -58     
- Partials      438      498      +60
Impacted Files Coverage Δ
app/react/src/server/build.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 49.42% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.47% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
app/react/src/server/babel_config.js 44.82% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
.../src/manager/containers/CommentsPanel/dataStore.js 34.78% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
... and 26 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 da6bd65...50aac05. Read the comment docs.

@shilman shilman merged commit afc12f9 into master Jun 28, 2017
@joscha joscha deleted the joscha/error-non-zero-on-stats-errors branch June 28, 2017 11:57
@joscha joscha changed the title fix: Fail on transpilation errors Exit storybook build non-zero on stats errors (e.g. errors in the transpilation pipeline) Jun 28, 2017
@joscha
Copy link
Member Author

joscha commented Jun 28, 2017

Just tested 3.1.7, works like a charm:

@storybook/react v3.1.7

=> Loading custom addons config.
=> Loading custom webpack config (extending mode).
Building storybook ...
ts-loader: Using typescript@2.3.4 and /Users/joscha/Development/canva/web/tsconfig.ts-loader.json
Failed to build the storybook
./src/pages/editor/publish/website/website_form_ui.tsx
(20,2): error TS2345: Argument of type 'typeof WebsiteFormUI' is not assignable to parameter of type 'string[]'.
  Property 'find' is missing in type 'typeof WebsiteFormUI'.
./src/pages/editor/publish/website/stories/website_form_ui.stories.tsx
(10,7): error TS2605: JSX element type 'WebsiteFormUI' is not a constructor function for JSX elements.
  Types of property 'setState' are incompatible.
    Type '{ <K extends never>(f: (prevState: void, props: WebsiteFormUIProps) => Pick<void, K>, callback?: (() => any) | undefined): void; <K extends never>(state: Pick<void, K>, callback?: (() => any) | undefined): void; }' is not assignable to type '{ <K extends never>(f: (prevState: {}, props: any) => Pick<{}, K>, callback?: (() => any) | undefined): void; <K extends never>(state: Pick<{}, K>, callback?: (() => any) | undefined): void; }'.
      Types of parameters 'f' and 'f' are incompatible.
        Type '(prevState: {}, props: any) => Pick<{}, any>' is not assignable to type '(prevState: void, props: WebsiteFormUIProps) => Pick<void, any>'.
          Types of parameters 'prevState' and 'prevState' are incompatible.
            Type 'void' is not assignable to type '{}'.
./src/pages/editor/publish/website/stories/website_form_ui.stories.tsx
(13,7): error TS2605: JSX element type 'WebsiteFormUI' is not a constructor function for JSX elements.
./src/pages/editor/publish/website/stories/website_form_ui.stories.tsx
(16,7): error TS2605: JSX element type 'WebsiteFormUI' is not a constructor function for JSX elements.
error Command failed with exit code 1.

@mrmartineau
Copy link
Contributor

@joscha @shilman I'm not sure, but does this change affect #1402? storybook-build does not work for quite a few people since version 3.1.7. Any thoughts?

@joscha
Copy link
Member Author

joscha commented Jul 25, 2017

@mrmartineau it could have been a hidden issue before, if the error was swallowed and never printed and also not changed the exit code. The changes in this PR would pick up such a hidden problem.

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.

4 participants