From 23f083a19b43be3bfded194d9a825654bc625773 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 16:42:14 +0900 Subject: [PATCH 1/8] Fail Storybook build if there are webpack compilation warnings Create a Webpack plugin which looks for warnings at the end of the build. Reference doc: https://webpack.js.org/contribute/writing-a-plugin/ --- packages/storybook/.storybook/main.js | 4 +++ .../webpack-plugin-fail-build-on-warning.js | 30 +++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js diff --git a/packages/storybook/.storybook/main.js b/packages/storybook/.storybook/main.js index c187dc5f56..cf003d0981 100644 --- a/packages/storybook/.storybook/main.js +++ b/packages/storybook/.storybook/main.js @@ -1,4 +1,5 @@ const path = require('path'); +const WebpackPluginFailBuildOnWarning = require('./webpack-plugin-fail-build-on-warning'); module.exports = { stories: [ @@ -53,6 +54,9 @@ module.exports = { ({ constructor }) => constructor.name !== 'ProgressPlugin', ); + // Fail storybook build if there are webpack warnings + config.plugins.push(new WebpackPluginFailBuildOnWarning()) + return config; }, }; diff --git a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js new file mode 100644 index 0000000000..4359035161 --- /dev/null +++ b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js @@ -0,0 +1,30 @@ +/* + * Copyright 2020 Spotify AB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +class WebpackPluginFailBuildOnWarning { + apply(compiler) { + compiler.hooks.done.tap('FailBuildOnWarning', stats => { + if (stats.compilation.warnings.length > 0) { + process.on('beforeExit', () => { + console.log(`You have ${stats.compilation.warnings.length} warning(s) in your webpack build. Exiting process as error.`) + process.exit(1); + }); + } + }) + } +} + +module.exports = WebpackPluginFailBuildOnWarning; From 706821337e05fecf0b2f60086a8791f12bc98261 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 16:50:27 +0900 Subject: [PATCH 2/8] Allow local storybook build even if there are warnings --- packages/storybook/.storybook/main.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/storybook/.storybook/main.js b/packages/storybook/.storybook/main.js index cf003d0981..12b9c34435 100644 --- a/packages/storybook/.storybook/main.js +++ b/packages/storybook/.storybook/main.js @@ -54,8 +54,10 @@ module.exports = { ({ constructor }) => constructor.name !== 'ProgressPlugin', ); - // Fail storybook build if there are webpack warnings - config.plugins.push(new WebpackPluginFailBuildOnWarning()) + // Fail storybook build on CI if there are webpack warnings. + if (process.env.CI) { + config.plugins.push(new WebpackPluginFailBuildOnWarning()) + } return config; }, From f3f1cdb063491a2638d08ff54f7ad3d70e5a8fbe Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 16:57:15 +0900 Subject: [PATCH 3/8] Temp commit to trigger Storybook build warning --- packages/core/src/components/Table/index.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core/src/components/Table/index.ts b/packages/core/src/components/Table/index.ts index 7ea3ead808..e85aac3a01 100644 --- a/packages/core/src/components/Table/index.ts +++ b/packages/core/src/components/Table/index.ts @@ -14,6 +14,5 @@ * limitations under the License. */ -export { default } from './Table'; -export type { TableColumn } from './Table'; +export { default, TableColumn } from './Table'; export { default as SubvalueCell } from './SubvalueCell'; From d050b1c79175bb1c38d4d55bb383402f52ee6d84 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 17:10:14 +0900 Subject: [PATCH 4/8] Fix storybook webpack warning because of TS types --- packages/core/src/components/Table/index.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/components/Table/index.ts b/packages/core/src/components/Table/index.ts index e85aac3a01..7ea3ead808 100644 --- a/packages/core/src/components/Table/index.ts +++ b/packages/core/src/components/Table/index.ts @@ -14,5 +14,6 @@ * limitations under the License. */ -export { default, TableColumn } from './Table'; +export { default } from './Table'; +export type { TableColumn } from './Table'; export { default as SubvalueCell } from './SubvalueCell'; From a984c9cf22258b2e2fab715a3a671ed77e63cede Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 19:02:37 +0900 Subject: [PATCH 5/8] Ignore a set of warnings during storybook webpack build --- .../webpack-plugin-fail-build-on-warning.js | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js index 4359035161..c00d1099b3 100644 --- a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js +++ b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js @@ -15,15 +15,37 @@ */ class WebpackPluginFailBuildOnWarning { + // Ignore the following warnings in the Webpack build. + warningsWhitelist = new Set([ + 'AssetsOverSizeLimitWarning', + 'EntrypointsOverSizeLimitWarning', + 'NoAsyncChunksWarning', + ]); + + /* Entry point for the Webpack plugin. */ apply(compiler) { - compiler.hooks.done.tap('FailBuildOnWarning', stats => { - if (stats.compilation.warnings.length > 0) { - process.on('beforeExit', () => { - console.log(`You have ${stats.compilation.warnings.length} warning(s) in your webpack build. Exiting process as error.`) - process.exit(1); - }); + // Invoke plugin logic when Webpack build is 'done'. + compiler.hooks.done.tap('FailBuildOnWarning', this.execute.bind(this)); + } + + execute(stats) { + // All the compilation warnings are stored in stats.compilation.warnings + let warnings = stats.compilation.warnings; + if (warnings.length > 0) { + // Throw error if there are unexpected warnings. + for (let warning of warnings) { + if (warning.name in this.warningsWhitelist) { + process.on('beforeExit', () => { + console.log( + `You have some unexpected warning(s) in your webpack build. Exiting process as error.`, + ); + process.exit(1); + }); + // No need to go over the rest of warnings from here. + break; + } } - }) + } } } From b2b91013eb7dc73dec12a83a06c91419370d2797 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 20:16:13 +0900 Subject: [PATCH 6/8] Fix bug in warnings whitelist check --- .../.storybook/webpack-plugin-fail-build-on-warning.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js index c00d1099b3..e814becd26 100644 --- a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js +++ b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js @@ -34,7 +34,7 @@ class WebpackPluginFailBuildOnWarning { if (warnings.length > 0) { // Throw error if there are unexpected warnings. for (let warning of warnings) { - if (warning.name in this.warningsWhitelist) { + if (!(warning.name in this.warningsWhitelist)) { process.on('beforeExit', () => { console.log( `You have some unexpected warning(s) in your webpack build. Exiting process as error.`, From f40ccc38a4525bc559b33f2ec779d770df1d2d90 Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 20:25:45 +0900 Subject: [PATCH 7/8] Fix syntax to check if an element is in a set --- .../.storybook/webpack-plugin-fail-build-on-warning.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js index e814becd26..d4a6f2dab3 100644 --- a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js +++ b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js @@ -34,7 +34,7 @@ class WebpackPluginFailBuildOnWarning { if (warnings.length > 0) { // Throw error if there are unexpected warnings. for (let warning of warnings) { - if (!(warning.name in this.warningsWhitelist)) { + if (!this.warningsWhitelist.has(warning.name)) { process.on('beforeExit', () => { console.log( `You have some unexpected warning(s) in your webpack build. Exiting process as error.`, From a835294a705b484d752cdcd97df3d46247fe950a Mon Sep 17 00:00:00 2001 From: Himanshu Mishra Date: Tue, 5 May 2020 22:49:27 +0900 Subject: [PATCH 8/8] Add comment about Webpack plugin to fail builds on warnings --- .../webpack-plugin-fail-build-on-warning.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js index d4a6f2dab3..1ccf825839 100644 --- a/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js +++ b/packages/storybook/.storybook/webpack-plugin-fail-build-on-warning.js @@ -14,6 +14,22 @@ * limitations under the License. */ +/** + * When building storybook, we can have warnings which may cause issues in the future. One of the example case is + * https://github.com/spotify/backstage/issues/718. To make sure new warnings are not introduced with new PRs, we + * want to fail CI builds if there are warnings when building storybook. + * + * This webpack plugin makes sure the CI builds fail on Webpack warnings. We also have a whitelist of warnings here + * which we think are non-critical. + * + * Note that this implementation will not detect other warnings emitted by storybook build that are separate from + * Webpack. A better solution over this plugin should be preferred, possibly on Storybook level (CLI options etc.) + * + * The case with #718 is caused because we are using `ts-loader` for `webpack` to load all our JS/TS files, but we + * have disabled type checking during build. This is done by setting `transpileOnly` to `true` in storybook/main.js + * and it improves the Storybook build speed. Because of this, Webpack emits warnings when we try to re-export types. + * Reference: https://github.com/TypeStrong/ts-loader#transpileonly + */ class WebpackPluginFailBuildOnWarning { // Ignore the following warnings in the Webpack build. warningsWhitelist = new Set([