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

Do not serve legacy JS when serving a Kibana Platform application #61011

Merged
merged 23 commits into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7af9555
Remove legacy imports from core/public
joshdover Mar 23, 2020
76f0ee8
Add core bundle to @kbn/optimizer
joshdover Mar 23, 2020
a37876f
Add core-only bundle loading strategy
joshdover Apr 7, 2020
4aba2c5
Update kbn-optimizer tests
joshdover Apr 8, 2020
ef11745
Rename standalone to entry, fix bootstrap
joshdover Apr 9, 2020
79c1a43
Avoid importing all of react-use in core
joshdover Apr 9, 2020
8cb723b
Merge remote-tracking branch 'upstream/master' into np/slim-bundle
joshdover Apr 10, 2020
21c3225
Merge branch 'master' into np/slim-bundle
elasticmachine Apr 13, 2020
b498623
Merge branch 'master' into np/slim-bundle
elasticmachine Apr 17, 2020
90938b7
Merge remote-tracking branch 'upstream/master' into np/slim-bundle
joshdover Apr 19, 2020
32c572a
PR comments
joshdover Apr 19, 2020
f9cc17d
Fix legacy CSS
joshdover Apr 19, 2020
af039ee
Fix optimizer test
joshdover Apr 19, 2020
6834f59
Rollback css changes
joshdover Apr 20, 2020
c7c6738
Fix optimizer tests for real
joshdover Apr 20, 2020
05ba7b1
Merge remote-tracking branch 'upstream/master' into np/slim-bundle
joshdover Apr 20, 2020
9b6d30a
Revert colon in plugin URL
joshdover Apr 20, 2020
5d893d6
Move legacy CSS again
joshdover Apr 20, 2020
329c044
Fix mixins import
joshdover Apr 20, 2020
fb44467
implement `includeCoreBundle: boolean` config
spalger Apr 20, 2020
1fd2e06
Merge pull request #30 from spalger/pr/61011
joshdover Apr 21, 2020
237e2c5
PR comments
joshdover Apr 21, 2020
6f648b1
Merge branch 'master' into np/slim-bundle
elasticmachine Apr 22, 2020
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
10 changes: 9 additions & 1 deletion packages/kbn-optimizer/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ run(
throw createFlagError('expected --cache to have no value');
}

const includeCoreBundle = flags.core ?? true;
if (typeof includeCoreBundle !== 'boolean') {
throw createFlagError('expected --core to have no value');
}

const dist = flags.dist ?? false;
if (typeof dist !== 'boolean') {
throw createFlagError('expected --dist to have no value');
Expand Down Expand Up @@ -87,6 +92,7 @@ run(
profileWebpack,
extraPluginScanDirs,
inspectWorkers,
includeCoreBundle,
});

await runOptimizer(config)
Expand All @@ -95,9 +101,10 @@ run(
},
{
flags: {
boolean: ['watch', 'oss', 'examples', 'dist', 'cache', 'profile', 'inspect-workers'],
boolean: ['core', 'watch', 'oss', 'examples', 'dist', 'cache', 'profile', 'inspect-workers'],
string: ['workers', 'scan-dir'],
default: {
core: true,
examples: true,
cache: true,
'inspect-workers': true,
Expand All @@ -107,6 +114,7 @@ run(
--workers max number of workers to use
--oss only build oss plugins
--profile profile the webpack builds and write stats.json files to build outputs
--no-core disable generating the core bundle
--no-cache disable the cache
--no-examples don't build the example plugins
--dist create bundles that are suitable for inclusion in the Kibana distributable
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-optimizer/src/common/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { BundleCache } from './bundle_cache';
import { UnknownVals } from './ts_helpers';
import { includes, ascending, entriesToObject } from './array_helpers';

const VALID_BUNDLE_TYPES = ['plugin' as const];
const VALID_BUNDLE_TYPES = ['plugin' as const, 'entry' as const];
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Are we planning to have additional entry type bundles? If not, wouldn't core be more explicit?

Copy link
Contributor

@spalger spalger Apr 15, 2020

Choose a reason for hiding this comment

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

I could imagine the login and logout pages being "entry" type bundles. The inclusion of plugins in those bundles has caused a number of issues over the years and it seems there needs not be a whole "platform" loaded to render those two pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OTOH, maybe it'd be better if the navigation after login was a SPA-style nav to the next app, which would be instant. Either way, I'm sort of indifferent here. I just used entry to make it more clear how this bundle could be used in a browser.

Copy link
Contributor

@spalger spalger Apr 20, 2020

Choose a reason for hiding this comment

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

I'm all about making login instant, it's really logout that has caused the most issues and it's also one of those pages that should really be as lightweight as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

The inclusion of plugins in those bundles has caused a number of issues over the years and it seems there needs not be a whole "platform" loaded to render those two pages.

Not sure it's feasible. That would add a limitation that security plugin cannot use other plugins API (licensing, for example) on login / logout pages. Also, it's already working with bootstrapping all the KP plugins.

Copy link
Contributor

@spalger spalger Apr 21, 2020

Choose a reason for hiding this comment

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

Also, it's already working with bootstrapping all the KP plugins.

There have been a handful of nasty race conditions caused by bootstrapping all the plugins on the logout page, which have lead to security vulnerabilities (all unshipped I believe), and having all that extra logic on the logout page currently serves no purpose.

I'm on board with "if it ain't broke, don't fix it", but I think this has broken enough times that we should look for solutions. Especially since the bugs have historically been "I clicked logout, and it said I was logged out, but when I refresh the login page I am logged in"


export interface BundleSpec {
readonly type: typeof VALID_BUNDLE_TYPES[0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@

import { createAbsolutePathSerializer } from '@kbn/dev-utils';

import { getBundles } from './get_bundles';
import { getPluginBundles } from './get_plugin_bundles';

expect.addSnapshotSerializer(createAbsolutePathSerializer('/repo'));

it('returns a bundle for each plugin', () => {
it('returns a bundle for core and each plugin', () => {
expect(
getBundles(
getPluginBundles(
[
{
directory: '/repo/plugins/foo',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { Bundle } from '../common';

import { KibanaPlatformPlugin } from './kibana_platform_plugins';

export function getBundles(plugins: KibanaPlatformPlugin[], repoRoot: string) {
export function getPluginBundles(plugins: KibanaPlatformPlugin[], repoRoot: string) {
return plugins
.filter(p => p.isUiPlugin)
.map(
Expand Down
28 changes: 22 additions & 6 deletions packages/kbn-optimizer/src/optimizer/optimizer_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

jest.mock('./assign_bundles_to_workers.ts');
jest.mock('./kibana_platform_plugins.ts');
jest.mock('./get_bundles.ts');
jest.mock('./get_plugin_bundles.ts');

import Path from 'path';
import Os from 'os';
Expand Down Expand Up @@ -90,6 +90,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": true,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 2,
"pluginPaths": Array [],
Expand All @@ -114,6 +115,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": false,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 2,
"pluginPaths": Array [],
Expand All @@ -138,6 +140,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": true,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 2,
"pluginPaths": Array [],
Expand All @@ -164,6 +167,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": true,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 2,
"pluginPaths": Array [],
Expand All @@ -187,6 +191,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": true,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 2,
"pluginPaths": Array [],
Expand All @@ -210,6 +215,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": true,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 100,
"pluginPaths": Array [],
Expand All @@ -230,6 +236,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": false,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 100,
"pluginPaths": Array [],
Expand All @@ -250,6 +257,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": false,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 100,
"pluginPaths": Array [],
Expand All @@ -271,6 +279,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": false,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 100,
"pluginPaths": Array [],
Expand All @@ -292,6 +301,7 @@ describe('OptimizerConfig::parseOptions()', () => {
Object {
"cache": true,
"dist": false,
"includeCoreBundle": false,
"inspectWorkers": false,
"maxWorkerCount": 100,
"pluginPaths": Array [],
Expand All @@ -314,7 +324,7 @@ describe('OptimizerConfig::create()', () => {
.assignBundlesToWorkers;
const findKibanaPlatformPlugins: jest.Mock = jest.requireMock('./kibana_platform_plugins.ts')
.findKibanaPlatformPlugins;
const getBundles: jest.Mock = jest.requireMock('./get_bundles.ts').getBundles;
const getPluginBundles: jest.Mock = jest.requireMock('./get_plugin_bundles.ts').getPluginBundles;

beforeEach(() => {
if ('mock' in OptimizerConfig.parseOptions) {
Expand All @@ -326,7 +336,7 @@ describe('OptimizerConfig::create()', () => {
{ config: Symbol('worker config 2') },
]);
findKibanaPlatformPlugins.mockReturnValue(Symbol('new platform plugins'));
getBundles.mockReturnValue(Symbol('bundles'));
getPluginBundles.mockReturnValue([Symbol('bundle1'), Symbol('bundle2')]);

jest.spyOn(OptimizerConfig, 'parseOptions').mockImplementation((): any => ({
cache: Symbol('parsed cache'),
Expand All @@ -348,7 +358,10 @@ describe('OptimizerConfig::create()', () => {

expect(config).toMatchInlineSnapshot(`
OptimizerConfig {
"bundles": Symbol(bundles),
"bundles": Array [
Symbol(bundle1),
Symbol(bundle2),
],
"cache": Symbol(parsed cache),
"dist": Symbol(parsed dist),
"inspectWorkers": Symbol(parsed inspect workers),
Expand Down Expand Up @@ -383,7 +396,7 @@ describe('OptimizerConfig::create()', () => {
}
`);

expect(getBundles.mock).toMatchInlineSnapshot(`
expect(getPluginBundles.mock).toMatchInlineSnapshot(`
Object {
"calls": Array [
Array [
Expand All @@ -400,7 +413,10 @@ describe('OptimizerConfig::create()', () => {
"results": Array [
Object {
"type": "return",
"value": Symbol(bundles),
"value": Array [
Symbol(bundle1),
Symbol(bundle2),
],
},
],
}
Expand Down
24 changes: 22 additions & 2 deletions packages/kbn-optimizer/src/optimizer/optimizer_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import Os from 'os';
import { Bundle, WorkerConfig } from '../common';

import { findKibanaPlatformPlugins, KibanaPlatformPlugin } from './kibana_platform_plugins';
import { getBundles } from './get_bundles';
import { getPluginBundles } from './get_plugin_bundles';

function pickMaxWorkerCount(dist: boolean) {
// don't break if cpus() returns nothing, or an empty array
Expand Down Expand Up @@ -60,6 +60,9 @@ interface Options {
pluginScanDirs?: string[];
/** absolute paths that should be added to the default scan dirs */
extraPluginScanDirs?: string[];

/** flag that causes the core bundle to be built along with plugins */
includeCoreBundle?: boolean;
}

interface ParsedOptions {
Expand All @@ -72,6 +75,7 @@ interface ParsedOptions {
pluginPaths: string[];
pluginScanDirs: string[];
inspectWorkers: boolean;
includeCoreBundle: boolean;
}

export class OptimizerConfig {
Expand All @@ -83,6 +87,7 @@ export class OptimizerConfig {
const profileWebpack = !!options.profileWebpack;
const inspectWorkers = !!options.inspectWorkers;
const cache = options.cache !== false && !process.env.KBN_OPTIMIZER_NO_CACHE;
const includeCoreBundle = !!options.includeCoreBundle;

const repoRoot = options.repoRoot;
if (!Path.isAbsolute(repoRoot)) {
Expand Down Expand Up @@ -134,13 +139,28 @@ export class OptimizerConfig {
pluginScanDirs,
pluginPaths,
inspectWorkers,
includeCoreBundle,
};
}

static create(inputOptions: Options) {
const options = OptimizerConfig.parseOptions(inputOptions);
const plugins = findKibanaPlatformPlugins(options.pluginScanDirs, options.pluginPaths);
const bundles = getBundles(plugins, options.repoRoot);
const bundles = [
...(options.includeCoreBundle
? [
new Bundle({
type: 'entry',
id: 'core',
entry: './public/entry_point',
sourceRoot: options.repoRoot,
contextDir: Path.resolve(options.repoRoot, 'src/core'),
outputDir: Path.resolve(options.repoRoot, 'src/core/target/public'),
}),
]
: []),
...getPluginBundles(plugins, options.repoRoot),
];

return new OptimizerConfig(
bundles,
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-optimizer/src/worker/webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export function getWebpackConfig(bundle: Bundle, worker: WorkerConfig) {

output: {
path: bundle.outputDir,
filename: '[name].plugin.js',
filename: `[name].${bundle.type}.js`,
publicPath: PUBLIC_PATH_PLACEHOLDER,
devtoolModuleFilenameTemplate: info =>
`/${bundle.type}:${bundle.id}/${Path.relative(
Expand Down
1 change: 1 addition & 0 deletions src/cli/cluster/run_kbn_optimizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export function runKbnOptimizer(opts: Record<string, any>, config: LegacyConfig)
const optimizerConfig = OptimizerConfig.create({
repoRoot: REPO_ROOT,
watch: true,
includeCoreBundle: true,
oss: !!opts.oss,
examples: !!opts.runExamples,
pluginPaths: config.get('plugins.paths'),
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion src/core/public/application/ui/app_router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import React, { FunctionComponent, useMemo } from 'react';
import { Route, RouteComponentProps, Router, Switch } from 'react-router-dom';
import { History } from 'history';
import { Observable } from 'rxjs';
import { useObservable } from 'react-use';
import useObservable from 'react-use/lib/useObservable';
joshdover marked this conversation as resolved.
Show resolved Hide resolved

import { AppLeaveHandler, AppStatus, Mounter } from '../types';
import { AppContainer } from './app_container';
Expand Down
12 changes: 7 additions & 5 deletions src/core/public/core_system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ const defaultCoreSystemParams = {
warnLegacyBrowsers: true,
},
} as any,
requireLegacyFiles: jest.fn(),
};

beforeEach(() => {
Expand Down Expand Up @@ -104,19 +103,22 @@ describe('constructor', () => {
});
});

it('passes requireLegacyFiles, useLegacyTestHarness, and a dom element to LegacyPlatformService', () => {
it('passes required params to LegacyPlatformService', () => {
const requireLegacyFiles = { requireLegacyFiles: true };
const useLegacyTestHarness = { useLegacyTestHarness: true };
const requireLegacyBootstrapModule = { requireLegacyBootstrapModule: true };
const requireNewPlatformShimModule = { requireNewPlatformShimModule: true };

createCoreSystem({
requireLegacyFiles,
useLegacyTestHarness,
requireLegacyBootstrapModule,
requireNewPlatformShimModule,
});

expect(LegacyPlatformServiceConstructor).toHaveBeenCalledTimes(1);
expect(LegacyPlatformServiceConstructor).toHaveBeenCalledWith({
requireLegacyFiles,
useLegacyTestHarness,
requireLegacyBootstrapModule,
requireNewPlatformShimModule,
});
});

Expand Down
13 changes: 7 additions & 6 deletions src/core/public/core_system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
* under the License.
*/

import './core.css';

import { CoreId } from '../server';
import { PackageInfo, EnvironmentMode } from '../server/types';
import { CoreSetup, CoreStart } from '.';
Expand Down Expand Up @@ -50,8 +48,9 @@ interface Params {
rootDomElement: HTMLElement;
browserSupportsCsp: boolean;
injectedMetadata: InjectedMetadataParams['injectedMetadata'];
requireLegacyFiles: LegacyPlatformParams['requireLegacyFiles'];
useLegacyTestHarness?: LegacyPlatformParams['useLegacyTestHarness'];
requireLegacyFiles?: LegacyPlatformParams['requireLegacyFiles'];
requireLegacyBootstrapModule?: LegacyPlatformParams['requireLegacyBootstrapModule'];
requireNewPlatformShimModule?: LegacyPlatformParams['requireNewPlatformShimModule'];
}

/** @internal */
Expand Down Expand Up @@ -111,7 +110,8 @@ export class CoreSystem {
browserSupportsCsp,
injectedMetadata,
requireLegacyFiles,
useLegacyTestHarness,
requireLegacyBootstrapModule,
requireNewPlatformShimModule,
} = params;

this.rootDomElement = rootDomElement;
Expand Down Expand Up @@ -145,7 +145,8 @@ export class CoreSystem {

this.legacy = new LegacyPlatformService({
requireLegacyFiles,
useLegacyTestHarness,
requireLegacyBootstrapModule,
requireNewPlatformShimModule,
});
}

Expand Down
Loading