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

[canvas][storybook] Improve Storybook Performance #34757

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/dev/precommit_hook/casing_check_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/


/**
* These patterns are used to identify files that are not supposed
* to be snake_case because their names are determined by other
Expand All @@ -43,22 +42,20 @@ export const IGNORE_FILE_GLOBS = [
'x-pack/docs/**/*',
'src/legacy/ui/public/assets/fonts/**/*',

// Files in this directory must match a pre-determined name in some cases.
'x-pack/plugins/canvas/.storybook/*',

// filename must match language code which requires capital letters
'**/translations/*.json'
'**/translations/*.json',
];


/**
* These patterns are matched against directories and indicate
* folders that must use kebab case.
*
* @type {Array}
*/
export const KEBAB_CASE_DIRECTORY_GLOBS = [
'packages/*',
'x-pack',
];

export const KEBAB_CASE_DIRECTORY_GLOBS = ['packages/*', 'x-pack'];

/**
* These patterns are matched against directories and indicate
Expand Down Expand Up @@ -88,7 +85,6 @@ export const IGNORE_DIRECTORY_GLOBS = [
'x-pack/dev-tools',
];


/**
* DO NOT ADD FILES TO THIS LIST!!
*
Expand Down
69 changes: 49 additions & 20 deletions src/dev/sass/build_sass.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,32 +22,25 @@ import { resolve } from 'path';
import { toArray } from 'rxjs/operators';

import { createFailError } from '../run';
import { findPluginSpecs } from '../../legacy/plugin_discovery';
import { collectUiExports } from '../../legacy/ui';
import { buildAll } from '../../legacy/server/sass/build_all';
import { findPluginSpecs } from '../../legacy/plugin_discovery';
import { collectUiExports } from '../../legacy/ui';
import { buildAll } from '../../legacy/server/sass/build_all';
import chokidar from 'chokidar';
import debounce from 'lodash/function/debounce';

export async function buildSass({ log, kibanaDir }) {
log.info('running plugin discovery in', kibanaDir);

const scanDirs = [
resolve(kibanaDir, 'src/legacy/core_plugins')
];

const paths = [ resolve(kibanaDir, 'x-pack') ];

const { spec$ } = findPluginSpecs({ plugins: { scanDirs, paths } });
const enabledPlugins = await spec$.pipe(toArray()).toPromise();
const uiExports = collectUiExports(enabledPlugins);
log.info('found %d styleSheetPaths', uiExports.styleSheetPaths.length);
log.verbose(uiExports.styleSheetPaths);
// TODO: clintandrewhall - Extract and use FSWatcher from legacy/server/sass
const build = async ({ log, kibanaDir, styleSheetPaths, watch }) => {
if (styleSheetPaths.length === 0) {
return;
}

let bundleCount = 0;
try {
const bundles = await buildAll({
styleSheets: uiExports.styleSheetPaths,
styleSheets: styleSheetPaths,
log,
buildDir: resolve(kibanaDir, 'built_assets/css'),
sourceMap: true
sourceMap: true,
});

bundles.forEach(bundle => {
Expand All @@ -60,5 +53,41 @@ export async function buildSass({ log, kibanaDir }) {
throw createFailError(`${message} on line ${line} of ${file}`);
}

log.success('%d scss bundles created', bundleCount);
log.success('%d scss bundles %s', bundleCount, watch ? 'rebuilt' : 'created');
};

export async function buildSass({ log, kibanaDir, watch }) {
log.info('running plugin discovery in', kibanaDir);

const scanDirs = [resolve(kibanaDir, 'src/legacy/core_plugins')];
const paths = [resolve(kibanaDir, 'x-pack')];
const { spec$ } = findPluginSpecs({ plugins: { scanDirs, paths } });
const enabledPlugins = await spec$.pipe(toArray()).toPromise();
const uiExports = collectUiExports(enabledPlugins);
const { styleSheetPaths } = uiExports;

log.info('%s %d styleSheetPaths', watch ? 'watching' : 'found', styleSheetPaths.length);
log.verbose(styleSheetPaths);

if (watch) {
const debouncedBuild = debounce(async path => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there supposed to be a wait defined for the debounce here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I'm not sure. I saw this methodology elsewhere in Kibana for a watch flag, so I took it as the best approach.

let buildPaths = styleSheetPaths;
if (path) {
buildPaths = styleSheetPaths.filter(styleSheetPath =>
path.includes(styleSheetPath.urlImports.publicDir)
);
}
await build({ log, kibanaDir, styleSheetPaths: buildPaths, watch });
});

const watchPaths = styleSheetPaths.map(styleSheetPath => styleSheetPath.urlImports.publicDir);

await build({ log, kibanaDir, styleSheetPaths });

chokidar.watch(watchPaths, { ignoreInitial: true }).on('all', (_, path) => {
debouncedBuild(path);
});
} else {
await build({ log, kibanaDir, styleSheetPaths });
}
}
41 changes: 24 additions & 17 deletions src/dev/sass/run_build_sass_cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,30 @@
*/

import { run } from '../run';
import { REPO_ROOT } from '../constants';
import { REPO_ROOT } from '../constants';
import { buildSass } from './build_sass';

run(async ({ log, flags: { kibanaDir } }) => {
await buildSass({
log,
kibanaDir
});
}, {
description: 'Simple CLI, useful for building scss files outside of the server',
flags: {
default: {
kibanaDir: REPO_ROOT
},
string: ['kibanaDir'],
help: `
--kibanaDir The root of the Kibana directory to build sass files in.
`
run(
async ({ log, flags: { kibanaDir, watch } }) => {
await buildSass({
log,
kibanaDir,
watch,
});
},
});
{
description: 'Simple CLI, useful for building scss files outside of the server',
flags: {
default: {
kibanaDir: REPO_ROOT,
watch: false,
},
string: ['kibanaDir'],
boolean: ['watch'],
help: `
--kibanaDir The root of the Kibana directory to build sass files in.
--watch Watch the SASS files and recompile them on save.
`,
},
}
);
4 changes: 2 additions & 2 deletions x-pack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"license": "Elastic-License",
"scripts": {
"kbn": "node ../scripts/kbn",
"kbn:bootstrap": "rm -rf ../built_assets/canvasStorybookDLL",
Copy link
Contributor

@tylersmalley tylersmalley Apr 10, 2019

Choose a reason for hiding this comment

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

Curious why is this necessary? Shouldn't it get overwritten when it's built?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to force a rebuild of the DLL only if the packages change. We don't want to rebuild the DLL on every startup, just when the dependencies are updated.

"start": "gulp dev",
"build": "gulp build",
"testonly": "gulp testonly",
Expand Down Expand Up @@ -119,8 +120,7 @@
"pdfjs-dist": "^2.0.943",
"pixelmatch": "4.0.2",
"proxyquire": "1.7.11",
"react-docgen-typescript-loader": "^3.0.0",
"react-docgen-typescript-webpack-plugin": "^1.1.0",
"react-docgen-typescript-loader": "^3.1.0",
"react-hooks-testing-library": "^0.3.8",
"react-test-renderer": "^16.8.0",
"react-testing-library": "^6.0.0",
Expand Down
22 changes: 8 additions & 14 deletions x-pack/plugins/canvas/.storybook/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@ import { withKnobs } from '@storybook/addon-knobs/react';
import { withInfo } from '@storybook/addon-info';
import { create } from '@storybook/theming';

// Import dependent CSS
require('@elastic/eui/dist/eui_theme_light.css');
require('@kbn/ui-framework/dist/kui_light.css');
require('../../../../src/legacy/ui/public/styles/bootstrap_light.less');

// If we're running Storyshots, be sure to register the require context hook.
// Otherwise, add the other decorators.
if (process.env.NODE_ENV === 'test') {
Expand All @@ -39,17 +34,16 @@ if (process.env.NODE_ENV === 'test') {
}

function loadStories() {
// Pull in the built CSS produced by the Kibana server
const css = require.context('../../../../built_assets/css', true, /light.css$/);
css.keys().forEach(filename => css(filename));
require('./dll_contexts');

// Include the legacy styles
const uiStyles = require.context(
'../../../../src/legacy/ui/public/styles',
false,
/[\/\\](?!mixins|variables|_|\.|bootstrap_(light|dark))[^\/\\]+\.less/
// Only gather and require CSS files related to Canvas. The other CSS files
// are built into the DLL.
const css = require.context(
'../../../../built_assets/css',
true,
/plugins\/(?=canvas).*light\.css/
);
uiStyles.keys().forEach(key => uiStyles(key));
css.keys().forEach(filename => css(filename));

// Find all files ending in *.examples.ts
const req = require.context('./..', true, /.examples.tsx$/);
Expand Down
19 changes: 19 additions & 0 deletions x-pack/plugins/canvas/.storybook/constants.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

const path = require('path');

const DLL_NAME = 'canvasStorybookDLL';
const KIBANA_ROOT = path.resolve(__dirname, '../../../..');
const BUILT_ASSETS = path.resolve(KIBANA_ROOT, 'built_assets');
const DLL_OUTPUT = path.resolve(BUILT_ASSETS, DLL_NAME);

module.exports = {
DLL_NAME,
KIBANA_ROOT,
BUILT_ASSETS,
DLL_OUTPUT,
};
28 changes: 28 additions & 0 deletions x-pack/plugins/canvas/.storybook/dll_contexts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

// This file defines CSS and Legacy style contexts for use in the DLL. This file
// is also require'd in the Storybook config so that the Storybook Webpack instance
// is aware of them, and can load them from the DLL.

// Pull in the built CSS produced by the Kibana server, but not
// the Canvas CSS-- we want that in the HMR service.
const css = require.context(
'../../../../built_assets/css',
true,
/\.\/plugins\/(?!canvas).*light\.css/
);
css.keys().forEach(filename => {
css(filename);
});

// Include Legacy styles
const uiStyles = require.context(
'../../../../src/legacy/ui/public/styles',
false,
/[\/\\](?!mixins|variables|_|\.|bootstrap_(light|dark))[^\/\\]+\.less/
);
uiStyles.keys().forEach(key => uiStyles(key));
6 changes: 3 additions & 3 deletions x-pack/plugins/canvas/.storybook/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
const serve = require('serve-static');
const path = require('path');

// Extend the Storybook Middleware to include a route to access ui assets
module.exports = function (router) {
// Extend the Storybook Middleware to include a route to access Legacy UI assets
module.exports = function(router) {
router.get('/ui', serve(path.resolve(__dirname, '../../../../src/legacy/ui/public/assets')));
}
};
6 changes: 6 additions & 0 deletions x-pack/plugins/canvas/.storybook/preview-head.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<!--
This file is looked for by Storybook and included in the HEAD element
if it exists. This is how we load the DLL content into the Storybook UI.
-->
<script src="/dll.js"></script>
<link href="/dll.css" rel="stylesheet" />
4 changes: 3 additions & 1 deletion x-pack/plugins/canvas/.storybook/storyshots.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
import path from 'path';
import initStoryshots, { multiSnapshotWithOptions } from '@storybook/addon-storyshots';
import styleSheetSerializer from 'jest-styled-components/src/styleSheetSerializer';
import { addSerializer } from 'jest-specific-snapshot'
import { addSerializer } from 'jest-specific-snapshot';

jest.mock(`@elastic/eui/lib/components/form/form_row/make_id`, () => () => `generated-id`);

addSerializer(styleSheetSerializer);

// Initialize Storyshots and build the Jest Snapshots
initStoryshots({
configPath: path.resolve(__dirname, './../.storybook'),
test: multiSnapshotWithOptions({}),
Expand Down
Loading