From 0f88c86628a11d042d9bfb9252e4c2535cff0bac Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Fri, 24 Apr 2020 11:11:48 +0200 Subject: [PATCH 01/19] convert into TS --- .../{bundles_route.js => bundles_route.ts} | 15 +++++++++- ..._response.js => dynamic_asset_response.ts} | 28 +++++++++++++------ ...ic_dirs.js => np_ui_plugin_public_dirs.ts} | 7 +++-- 3 files changed, 38 insertions(+), 12 deletions(-) rename src/optimize/bundles_route/{bundles_route.js => bundles_route.ts} (92%) rename src/optimize/bundles_route/{dynamic_asset_response.js => dynamic_asset_response.ts} (84%) rename src/optimize/{np_ui_plugin_public_dirs.js => np_ui_plugin_public_dirs.ts} (84%) diff --git a/src/optimize/bundles_route/bundles_route.js b/src/optimize/bundles_route/bundles_route.ts similarity index 92% rename from src/optimize/bundles_route/bundles_route.js rename to src/optimize/bundles_route/bundles_route.ts index 4030988c8552c..43b98a51f436e 100644 --- a/src/optimize/bundles_route/bundles_route.js +++ b/src/optimize/bundles_route/bundles_route.ts @@ -18,6 +18,7 @@ */ import { isAbsolute, extname, join } from 'path'; +import Hapi from 'hapi'; import LruCache from 'lru-cache'; import * as UiSharedDeps from '@kbn/ui-shared-deps'; import { createDynamicAssetResponse } from './dynamic_asset_response'; @@ -44,6 +45,12 @@ export function createBundlesRoute({ basePublicPath, builtCssPath, npUiPluginPublicDirs = [], +}: { + regularBundlesPath: string; + dllBundlesPath: string; + basePublicPath: string; + builtCssPath: string; + npUiPluginPublicDirs?: any[]; }) { // rather than calculate the fileHash on every request, we // provide a cache object to `resolveDynamicAssetResponse()` that @@ -122,6 +129,12 @@ function buildRouteForBundles({ bundlesPath, fileHashCache, replacePublicPath = true, +}: { + publicPath: string; + routePath: string; + bundlesPath: string; + fileHashCache: LruCache; + replacePublicPath?: boolean; }) { return { method: 'GET', @@ -130,7 +143,7 @@ function buildRouteForBundles({ auth: false, ext: { onPreHandler: { - method(request, h) { + method(request: Hapi.Request, h: Hapi.ResponseToolkit) { const ext = extname(request.params.path); if (ext !== '.js' && ext !== '.css') { diff --git a/src/optimize/bundles_route/dynamic_asset_response.js b/src/optimize/bundles_route/dynamic_asset_response.ts similarity index 84% rename from src/optimize/bundles_route/dynamic_asset_response.js rename to src/optimize/bundles_route/dynamic_asset_response.ts index 80c49a26270fd..b61ea1225f8e7 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.js +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -20,10 +20,15 @@ import { resolve } from 'path'; import { open, fstat, createReadStream, close } from 'fs'; +import Hapi from 'hapi'; +import LruCache from 'lru-cache'; + import Boom from 'boom'; import { fromNode as fcb } from 'bluebird'; +// @ts-ignore import { getFileHash } from './file_hash'; +// @ts-ignore import { replacePlaceholder } from '../public_path_placeholder'; /** @@ -51,16 +56,23 @@ import { replacePlaceholder } from '../public_path_placeholder'; * @property {string} options.publicPath * @property {LruCache} options.fileHashCache */ -export async function createDynamicAssetResponse(options) { +export async function createDynamicAssetResponse(options: { + request: Hapi.Request; + h: Hapi.ResponseToolkit; + bundlesPath: string; + publicPath: string; + fileHashCache: LruCache; + replacePublicPath: any; +}) { const { request, h, bundlesPath, publicPath, fileHashCache, replacePublicPath } = options; - let fd; + let fd: number | undefined; try { const path = resolve(bundlesPath, request.params.path); // prevent path traversal, only process paths that resolve within bundlesPath if (!path.startsWith(bundlesPath)) { - throw Boom.forbidden(null, 'EACCES'); + throw Boom.forbidden(undefined, 'EACCES'); } // we use and manage a file descriptor mostly because @@ -68,15 +80,15 @@ export async function createDynamicAssetResponse(options) { // the file 2 or 3 times per request it seems logical fd = await fcb(cb => open(path, 'r', cb)); - const stat = await fcb(cb => fstat(fd, cb)); + const stat = await fcb(cb => fstat(fd!, cb)); const hash = await getFileHash(fileHashCache, path, stat, fd); - const read = createReadStream(null, { + const read = createReadStream(null as any, { fd, start: 0, autoClose: true, }); - fd = null; // read stream is now responsible for fd + fd = undefined; // read stream is now responsible for fd const content = replacePublicPath ? replacePlaceholder(read, publicPath) : read; const etag = replacePublicPath ? `${hash}-${publicPath}` : hash; @@ -91,8 +103,8 @@ export async function createDynamicAssetResponse(options) { } catch (error) { if (fd) { try { - await fcb(cb => close(fd, cb)); - } catch (error) { + await fcb(cb => close(fd!, cb)); + } catch (err) { // ignore errors from close, we already have one to report // and it's very likely they are the same } diff --git a/src/optimize/np_ui_plugin_public_dirs.js b/src/optimize/np_ui_plugin_public_dirs.ts similarity index 84% rename from src/optimize/np_ui_plugin_public_dirs.js rename to src/optimize/np_ui_plugin_public_dirs.ts index de05fd2b863b8..c96e411763dc1 100644 --- a/src/optimize/np_ui_plugin_public_dirs.js +++ b/src/optimize/np_ui_plugin_public_dirs.ts @@ -16,8 +16,9 @@ * specific language governing permissions and limitations * under the License. */ +import KbnServer from '../legacy/server/kbn_server'; -export function getNpUiPluginPublicDirs(kbnServer) { +export function getNpUiPluginPublicDirs(kbnServer: KbnServer) { return Array.from(kbnServer.newPlatform.__internals.uiPlugins.internal.entries()).map( ([id, { publicTargetDir }]) => ({ id, @@ -26,7 +27,7 @@ export function getNpUiPluginPublicDirs(kbnServer) { ); } -export function isNpUiPluginPublicDirs(something) { +export function isNpUiPluginPublicDirs(something: any) { return ( Array.isArray(something) && something.every( @@ -35,7 +36,7 @@ export function isNpUiPluginPublicDirs(something) { ); } -export function assertIsNpUiPluginPublicDirs(something) { +export function assertIsNpUiPluginPublicDirs(something: any) { if (!isNpUiPluginPublicDirs(something)) { throw new TypeError( 'npUiPluginPublicDirs must be an array of objects with string `id` and `path` properties' From 5c2bc55b3ba413247fda1ad6ab4a77d6887d04d3 Mon Sep 17 00:00:00 2001 From: restrry Date: Fri, 24 Apr 2020 12:55:54 +0200 Subject: [PATCH 02/19] load plugin scripts in html body --- src/core/public/plugins/plugin.test.ts | 15 -- src/core/public/plugins/plugin.ts | 48 ++++-- src/core/public/plugins/plugin_loader.mock.ts | 33 ---- src/core/public/plugins/plugin_loader.test.ts | 125 ---------------- src/core/public/plugins/plugin_loader.ts | 141 ------------------ .../plugins/plugins_service.test.mocks.ts | 6 - .../public/plugins/plugins_service.test.ts | 25 +--- src/core/public/plugins/plugins_service.ts | 8 - src/legacy/ui/ui_render/ui_render_mixin.js | 6 +- 9 files changed, 40 insertions(+), 367 deletions(-) delete mode 100644 src/core/public/plugins/plugin_loader.mock.ts delete mode 100644 src/core/public/plugins/plugin_loader.test.ts delete mode 100644 src/core/public/plugins/plugin_loader.ts diff --git a/src/core/public/plugins/plugin.test.ts b/src/core/public/plugins/plugin.test.ts index 39330711f7980..4ed2106d542d5 100644 --- a/src/core/public/plugins/plugin.test.ts +++ b/src/core/public/plugins/plugin.test.ts @@ -38,7 +38,6 @@ function createManifest( let plugin: PluginWrapper>; const opaqueId = Symbol(); const initializerContext = coreMock.createPluginInitializerContext(); -const addBasePath = (path: string) => path; beforeEach(() => { mockPluginLoader.mockClear(); @@ -49,11 +48,6 @@ beforeEach(() => { }); describe('PluginWrapper', () => { - test('`load` calls loadPluginBundle', () => { - plugin.load(addBasePath); - expect(mockPluginLoader).toHaveBeenCalledWith(addBasePath, 'plugin-a'); - }); - test('`setup` fails if load is not called first', async () => { await expect(plugin.setup({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot( `"Plugin \\"plugin-a\\" can't be setup since its bundle isn't loaded."` @@ -62,7 +56,6 @@ describe('PluginWrapper', () => { test('`setup` fails if plugin.setup is not a function', async () => { mockInitializer.mockReturnValueOnce({ start: jest.fn() } as any); - await plugin.load(addBasePath); await expect(plugin.setup({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot( `"Instance of plugin \\"plugin-a\\" does not define \\"setup\\" function."` ); @@ -70,20 +63,17 @@ describe('PluginWrapper', () => { test('`setup` fails if plugin.start is not a function', async () => { mockInitializer.mockReturnValueOnce({ setup: jest.fn() } as any); - await plugin.load(addBasePath); await expect(plugin.setup({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot( `"Instance of plugin \\"plugin-a\\" does not define \\"start\\" function."` ); }); test('`setup` calls initializer with initializer context', async () => { - await plugin.load(addBasePath); await plugin.setup({} as any, {} as any); expect(mockInitializer).toHaveBeenCalledWith(initializerContext); }); test('`setup` calls plugin.setup with context and dependencies', async () => { - await plugin.load(addBasePath); const context = { any: 'thing' } as any; const deps = { otherDep: 'value' }; await plugin.setup(context, deps); @@ -91,14 +81,12 @@ describe('PluginWrapper', () => { }); test('`start` fails if setup is not called first', async () => { - await plugin.load(addBasePath); await expect(plugin.start({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot( `"Plugin \\"plugin-a\\" can't be started since it isn't set up."` ); }); test('`start` calls plugin.start with context and dependencies', async () => { - await plugin.load(addBasePath); await plugin.setup({} as any, {} as any); const context = { any: 'thing' } as any; const deps = { otherDep: 'value' }; @@ -123,7 +111,6 @@ describe('PluginWrapper', () => { return pluginStartContract; }, })); - await plugin.load(addBasePath); await plugin.setup({} as any, {} as any); const context = { any: 'thing' } as any; const deps = { otherDep: 'value' }; @@ -145,7 +132,6 @@ describe('PluginWrapper', () => { }); test('`stop` calls plugin.stop', async () => { - await plugin.load(addBasePath); await plugin.setup({} as any, {} as any); await plugin.stop(); expect(mockPlugin.stop).toHaveBeenCalled(); @@ -153,7 +139,6 @@ describe('PluginWrapper', () => { test('`stop` does not fail if plugin.stop does not exist', async () => { mockInitializer.mockReturnValueOnce({ setup: jest.fn(), start: jest.fn() } as any); - await plugin.load(addBasePath); await plugin.setup({} as any, {} as any); expect(() => plugin.stop()).not.toThrow(); }); diff --git a/src/core/public/plugins/plugin.ts b/src/core/public/plugins/plugin.ts index e51c45040c452..8957b7f4f1508 100644 --- a/src/core/public/plugins/plugin.ts +++ b/src/core/public/plugins/plugin.ts @@ -21,9 +21,24 @@ import { Subject } from 'rxjs'; import { first } from 'rxjs/operators'; import { DiscoveredPlugin, PluginOpaqueId } from '../../server'; import { PluginInitializerContext } from './plugin_context'; -import { loadPluginBundle } from './plugin_loader'; import { CoreStart, CoreSetup } from '..'; +/** + * Unknown variant for internal use only for when plugins are not known. + * @internal + */ +export type UnknownPluginInitializer = PluginInitializer>; + +/** + * Custom window type for loading bundles. Do not extend global Window to avoid leaking these types. + * @internal + */ +export interface CoreWindow { + __kbnBundles__: { + [pluginBundleName: string]: { plugin: UnknownPluginInitializer } | undefined; + }; +} + /** * The interface that should be returned by a `PluginInitializer`. * @@ -69,7 +84,6 @@ export class PluginWrapper< public readonly configPath: DiscoveredPlugin['configPath']; public readonly requiredPlugins: DiscoveredPlugin['requiredPlugins']; public readonly optionalPlugins: DiscoveredPlugin['optionalPlugins']; - private initializer?: PluginInitializer; private instance?: Plugin; private readonly startDependencies$ = new Subject<[CoreStart, TPluginsStart, TStart]>(); @@ -87,15 +101,22 @@ export class PluginWrapper< } /** - * Loads the plugin's bundle into the browser. Should be called in parallel with all plugins - * using `Promise.all`. Must be called before `setup`. - * @param addBasePath Function that adds the base path to a string for plugin bundle path. + * Reads the plugin's bundle declared in the global context. */ - public async load(addBasePath: (path: string) => string) { - this.initializer = await loadPluginBundle( - addBasePath, - this.name - ); + private read() { + const coreWindow = (window as unknown) as CoreWindow; + const exportId = `plugin/${this.name}`; + const pluginExport = coreWindow.__kbnBundles__[exportId]; + if (typeof pluginExport!.plugin !== 'function') { + throw new Error(`Definition of plugin "${this.name}" should be a function.`); + } else { + return pluginExport!.plugin as PluginInitializer< + TSetup, + TStart, + TPluginsSetup, + TPluginsStart + >; + } } /** @@ -146,11 +167,8 @@ export class PluginWrapper< } private async createPluginInstance() { - if (this.initializer === undefined) { - throw new Error(`Plugin "${this.name}" can't be setup since its bundle isn't loaded.`); - } - - const instance = this.initializer(this.initializerContext); + const initializer = this.read(); + const instance = initializer(this.initializerContext); if (typeof instance.setup !== 'function') { throw new Error(`Instance of plugin "${this.name}" does not define "setup" function.`); diff --git a/src/core/public/plugins/plugin_loader.mock.ts b/src/core/public/plugins/plugin_loader.mock.ts deleted file mode 100644 index abdd9d4ddce2a..0000000000000 --- a/src/core/public/plugins/plugin_loader.mock.ts +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you 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. - */ - -import { PluginName } from 'src/core/server'; -import { LoadPluginBundle, UnknownPluginInitializer } from './plugin_loader'; - -/** - * @param initializerProvider A function provided by the test to resolve initializers. - */ -const createLoadPluginBundleMock = ( - initializerProvider: (name: PluginName) => UnknownPluginInitializer -): jest.Mock, Parameters> => - jest.fn((addBasePath, pluginName, _ = {}) => { - return Promise.resolve(initializerProvider(pluginName)) as any; - }); - -export const loadPluginBundleMock = { create: createLoadPluginBundleMock }; diff --git a/src/core/public/plugins/plugin_loader.test.ts b/src/core/public/plugins/plugin_loader.test.ts deleted file mode 100644 index b4e2c3095f14a..0000000000000 --- a/src/core/public/plugins/plugin_loader.test.ts +++ /dev/null @@ -1,125 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you 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. - */ - -import { CoreWindow, loadPluginBundle } from './plugin_loader'; - -let createdScriptTags = [] as any[]; -let appendChildSpy: jest.SpyInstance; -let createElementSpy: jest.SpyInstance< - HTMLElement, - [string, (ElementCreationOptions | undefined)?] ->; - -const coreWindow = (window as unknown) as CoreWindow; - -beforeEach(() => { - // Mock document.createElement to return fake tags we can use to inspect what - // loadPluginBundles does. - createdScriptTags = []; - createElementSpy = jest.spyOn(document, 'createElement').mockImplementation(() => { - const scriptTag = { setAttribute: jest.fn() } as any; - createdScriptTags.push(scriptTag); - return scriptTag; - }); - - // Mock document.body.appendChild to avoid errors about appending objects that aren't `Node`'s - // and so we can verify that the script tags were added to the page. - appendChildSpy = jest.spyOn(document.body, 'appendChild').mockReturnValue({} as any); - - // Mock global fields needed for loading modules. - coreWindow.__kbnBundles__ = {}; -}); - -afterEach(() => { - appendChildSpy.mockRestore(); - createElementSpy.mockRestore(); - delete coreWindow.__kbnBundles__; -}); - -const addBasePath = (path: string) => path; - -test('`loadPluginBundles` creates a script tag and loads initializer', async () => { - const loadPromise = loadPluginBundle(addBasePath, 'plugin-a'); - - // Verify it sets up the script tag correctly and adds it to document.body - expect(createdScriptTags).toHaveLength(1); - const fakeScriptTag = createdScriptTags[0]; - expect(fakeScriptTag.setAttribute).toHaveBeenCalledWith( - 'src', - '/bundles/plugin/plugin-a/plugin-a.plugin.js' - ); - expect(fakeScriptTag.setAttribute).toHaveBeenCalledWith('id', 'kbn-plugin-plugin-a'); - expect(fakeScriptTag.onload).toBeInstanceOf(Function); - expect(fakeScriptTag.onerror).toBeInstanceOf(Function); - expect(appendChildSpy).toHaveBeenCalledWith(fakeScriptTag); - - // Setup a fake initializer as if a plugin bundle had actually been loaded. - const fakeInitializer = jest.fn(); - coreWindow.__kbnBundles__['plugin/plugin-a'] = { plugin: fakeInitializer }; - // Call the onload callback - fakeScriptTag.onload(); - await expect(loadPromise).resolves.toEqual(fakeInitializer); -}); - -test('`loadPluginBundles` includes the basePath', async () => { - loadPluginBundle((path: string) => `/mybasepath${path}`, 'plugin-a'); - - // Verify it sets up the script tag correctly and adds it to document.body - expect(createdScriptTags).toHaveLength(1); - const fakeScriptTag = createdScriptTags[0]; - expect(fakeScriptTag.setAttribute).toHaveBeenCalledWith( - 'src', - '/mybasepath/bundles/plugin/plugin-a/plugin-a.plugin.js' - ); -}); - -test('`loadPluginBundles` rejects if script.onerror is called', async () => { - const loadPromise = loadPluginBundle(addBasePath, 'plugin-a'); - const fakeScriptTag1 = createdScriptTags[0]; - // Call the error on the second script - fakeScriptTag1.onerror(new Error('Whoa there!')); - - await expect(loadPromise).rejects.toThrowErrorMatchingInlineSnapshot( - `"Failed to load \\"plugin-a\\" bundle (/bundles/plugin/plugin-a/plugin-a.plugin.js)"` - ); -}); - -test('`loadPluginBundles` rejects if timeout is reached', async () => { - await expect( - // Override the timeout to 1 ms for testi. - loadPluginBundle(addBasePath, 'plugin-a', { timeoutMs: 1 }) - ).rejects.toThrowErrorMatchingInlineSnapshot( - `"Timeout reached when loading \\"plugin-a\\" bundle (/bundles/plugin/plugin-a/plugin-a.plugin.js)"` - ); -}); - -test('`loadPluginBundles` rejects if bundle does attach an initializer to window.__kbnBundles__', async () => { - const loadPromise = loadPluginBundle(addBasePath, 'plugin-a'); - - const fakeScriptTag1 = createdScriptTags[0]; - - // Setup a fake initializer as if a plugin bundle had actually been loaded. - coreWindow.__kbnBundles__['plugin/plugin-a'] = undefined; - // Call the onload callback - fakeScriptTag1.onload(); - - await expect(loadPromise).rejects.toThrowErrorMatchingInlineSnapshot( - `"Definition of plugin \\"plugin-a\\" should be a function (/bundles/plugin/plugin-a/plugin-a.plugin.js)."` - ); -}); diff --git a/src/core/public/plugins/plugin_loader.ts b/src/core/public/plugins/plugin_loader.ts deleted file mode 100644 index bf7711055e97b..0000000000000 --- a/src/core/public/plugins/plugin_loader.ts +++ /dev/null @@ -1,141 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you 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. - */ - -import { PluginName } from '../../server'; -import { PluginInitializer } from './plugin'; - -/** - * Unknown variant for internal use only for when plugins are not known. - * @internal - */ -export type UnknownPluginInitializer = PluginInitializer>; - -/** - * Custom window type for loading bundles. Do not extend global Window to avoid leaking these types. - * @internal - */ -export interface CoreWindow { - __kbnBundles__: { - [pluginBundleName: string]: { plugin: UnknownPluginInitializer } | undefined; - }; -} - -/** - * Timeout for loading a single script in milliseconds. - * @internal - */ -export const LOAD_TIMEOUT = 120 * 1000; // 2 minutes - -/** - * Loads the bundle for a plugin onto the page and returns their PluginInitializer. This should - * be called for all plugins (once per plugin) in parallel using Promise.all. - * - * If this is slowing down browser load time, there are some ways we could make this faster: - * - Add these bundles in the generated bootstrap.js file so they're loaded immediately - * - Concatenate all the bundles files on the backend and serve them in single request. - * - Use HTTP/2 to load these bundles without having to open new connections for each. - * - * This may not be much of an issue since these should be cached by the browser after the first - * page load. - * - * @param basePath - * @param plugins - * @internal - */ -export const loadPluginBundle: LoadPluginBundle = < - TSetup, - TStart, - TPluginsSetup extends object, - TPluginsStart extends object ->( - addBasePath: (path: string) => string, - pluginName: PluginName, - { timeoutMs = LOAD_TIMEOUT }: { timeoutMs?: number } = {} -) => - new Promise>( - (resolve, reject) => { - const coreWindow = (window as unknown) as CoreWindow; - const exportId = `plugin/${pluginName}`; - - const readPluginExport = () => { - const PluginExport: any = coreWindow.__kbnBundles__[exportId]; - if (typeof PluginExport?.plugin !== 'function') { - reject( - new Error(`Definition of plugin "${pluginName}" should be a function (${bundlePath}).`) - ); - } else { - resolve( - PluginExport.plugin as PluginInitializer - ); - } - }; - - if (coreWindow.__kbnBundles__[exportId]) { - readPluginExport(); - return; - } - - const script = document.createElement('script'); - // Assumes that all plugin bundles get put into the bundles/plugins subdirectory - const bundlePath = addBasePath(`/bundles/plugin/${pluginName}/${pluginName}.plugin.js`); - script.setAttribute('src', bundlePath); - script.setAttribute('id', `kbn-plugin-${pluginName}`); - script.setAttribute('async', ''); - - const cleanupTag = () => { - clearTimeout(timeout); - // Set to null for IE memory leak issue. Webpack does the same thing. - // @ts-ignore - script.onload = script.onerror = null; - }; - - // Wire up resolve and reject - script.onload = () => { - cleanupTag(); - readPluginExport(); - }; - - script.onerror = () => { - cleanupTag(); - reject(new Error(`Failed to load "${pluginName}" bundle (${bundlePath})`)); - }; - - const timeout = setTimeout(() => { - cleanupTag(); - reject(new Error(`Timeout reached when loading "${pluginName}" bundle (${bundlePath})`)); - }, timeoutMs); - - // Add the script tag to the end of the body to start downloading - document.body.appendChild(script); - } - ); - -/** - * @internal - */ -export type LoadPluginBundle = < - TSetup, - TStart, - TPluginsSetup extends object, - TPluginsStart extends object ->( - addBasePath: (path: string) => string, - pluginName: PluginName, - options?: { timeoutMs?: number } -) => Promise>; diff --git a/src/core/public/plugins/plugins_service.test.mocks.ts b/src/core/public/plugins/plugins_service.test.mocks.ts index a76078932518f..46879c8133366 100644 --- a/src/core/public/plugins/plugins_service.test.mocks.ts +++ b/src/core/public/plugins/plugins_service.test.mocks.ts @@ -19,7 +19,6 @@ import { PluginName } from 'kibana/server'; import { Plugin } from './plugin'; -import { loadPluginBundleMock } from './plugin_loader.mock'; export type MockedPluginInitializer = jest.Mock>, any>; @@ -27,8 +26,3 @@ export const mockPluginInitializerProvider: jest.Mock< MockedPluginInitializer, [PluginName] > = jest.fn().mockRejectedValue(new Error('No provider specified')); - -export const mockLoadPluginBundle = loadPluginBundleMock.create(mockPluginInitializerProvider); -jest.mock('./plugin_loader', () => ({ - loadPluginBundle: mockLoadPluginBundle, -})); diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index 688eaf4f2bfc7..d8b627747e4df 100644 --- a/src/core/public/plugins/plugins_service.test.ts +++ b/src/core/public/plugins/plugins_service.test.ts @@ -21,7 +21,6 @@ import { omit, pick } from 'lodash'; import { MockedPluginInitializer, - mockLoadPluginBundle, mockPluginInitializerProvider, } from './plugins_service.test.mocks'; @@ -153,7 +152,7 @@ describe('PluginsService', () => { }); afterEach(() => { - mockLoadPluginBundle.mockClear(); + // mockLoadPluginBundle.mockClear(); }); describe('#getOpaqueIds()', () => { @@ -175,7 +174,8 @@ describe('PluginsService', () => { describe('#setup()', () => { it('fails if any bundle cannot be loaded', async () => { - mockLoadPluginBundle.mockRejectedValueOnce(new Error('Could not load bundle')); + // TODO + // mockLoadPluginBundle.mockRejectedValueOnce(new Error('Could not load bundle')); const pluginsService = new PluginsService(mockCoreContext, plugins); await expect(pluginsService.setup(mockSetupDeps)).rejects.toThrowErrorMatchingInlineSnapshot( @@ -191,25 +191,6 @@ describe('PluginsService', () => { ); }); - it('calls loadPluginBundles with http and plugins', async () => { - const pluginsService = new PluginsService(mockCoreContext, plugins); - await pluginsService.setup(mockSetupDeps); - - expect(mockLoadPluginBundle).toHaveBeenCalledTimes(3); - expect(mockLoadPluginBundle).toHaveBeenCalledWith( - mockSetupDeps.http.basePath.prepend, - 'pluginA' - ); - expect(mockLoadPluginBundle).toHaveBeenCalledWith( - mockSetupDeps.http.basePath.prepend, - 'pluginB' - ); - expect(mockLoadPluginBundle).toHaveBeenCalledWith( - mockSetupDeps.http.basePath.prepend, - 'pluginC' - ); - }); - it('initializes plugins with PluginInitializerContext', async () => { const pluginsService = new PluginsService(mockCoreContext, plugins); await pluginsService.setup(mockSetupDeps); diff --git a/src/core/public/plugins/plugins_service.ts b/src/core/public/plugins/plugins_service.ts index e698af689036d..862aa5043ad4b 100644 --- a/src/core/public/plugins/plugins_service.ts +++ b/src/core/public/plugins/plugins_service.ts @@ -93,9 +93,6 @@ export class PluginsService implements CoreService { - // Load plugin bundles - await this.loadPluginBundles(deps.http.basePath.prepend); - // Setup each plugin with required and optional plugin contracts const contracts = new Map(); for (const [pluginName, plugin] of this.plugins.entries()) { @@ -167,9 +164,4 @@ export class PluginsService implements CoreService string) { - // Load all bundles in parallel - return Promise.all([...this.plugins.values()].map(plugin => plugin.load(addBasePath))); - } } diff --git a/src/legacy/ui/ui_render/ui_render_mixin.js b/src/legacy/ui/ui_render/ui_render_mixin.js index 801eecf5b608b..fa28b5c8d698c 100644 --- a/src/legacy/ui/ui_render/ui_render_mixin.js +++ b/src/legacy/ui/ui_render/ui_render_mixin.js @@ -135,6 +135,7 @@ export function uiRenderMixin(kbnServer, server, config) { .reverse(), ]), ]; + const uiPluginIds = [...kbnServer.newPlatform.__internals.uiPlugins.public.keys()]; const jsDependencyPaths = [ ...UiSharedDeps.jsDepFilenames.map( @@ -151,10 +152,11 @@ export function uiRenderMixin(kbnServer, server, config) { `${regularBundlePath}/plugin/kibanaUtils/kibanaUtils.plugin.js`, `${regularBundlePath}/plugin/esUiShared/esUiShared.plugin.js`, `${regularBundlePath}/plugin/kibanaReact/kibanaReact.plugin.js`, + ...uiPluginIds.map( + pluginId => `${regularBundlePath}/plugin/${pluginId}/${pluginId}.plugin.js` + ), ]; - const uiPluginIds = [...kbnServer.newPlatform.__internals.uiPlugins.public.keys()]; - // These paths should align with the bundle routes configured in // src/optimize/bundles_route/bundles_route.js const publicPathMap = JSON.stringify({ From 3ab5f29c703c2304e0c68ed119e412e02f1f429d Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Fri, 24 Apr 2020 22:19:40 +0200 Subject: [PATCH 03/19] use buildNum as a unique Id for cache busting --- src/legacy/server/kbn_server.d.ts | 1 + src/legacy/ui/ui_render/ui_render_mixin.js | 22 ++++++++--------- src/optimize/bundles_route/bundles_route.ts | 23 +++++++++++++++++- .../bundles_route/dynamic_asset_response.ts | 24 +++++++++++++++---- .../bundles_route/{index.js => index.ts} | 0 ...undles_route.js => proxy_bundles_route.ts} | 4 ++-- src/optimize/{index.js => index.ts} | 12 ++++++++-- src/optimize/watch/optmzr_role.js | 3 ++- src/optimize/watch/watch_optimizer.js | 3 ++- src/optimize/watch/watch_server.js | 10 ++++++-- 10 files changed, 78 insertions(+), 24 deletions(-) rename src/optimize/bundles_route/{index.js => index.ts} (100%) rename src/optimize/bundles_route/{proxy_bundles_route.js => proxy_bundles_route.ts} (87%) rename src/optimize/{index.js => index.ts} (85%) diff --git a/src/legacy/server/kbn_server.d.ts b/src/legacy/server/kbn_server.d.ts index 0d2f3528c9019..02491ff872981 100644 --- a/src/legacy/server/kbn_server.d.ts +++ b/src/legacy/server/kbn_server.d.ts @@ -153,6 +153,7 @@ export default class KbnServer { public server: Server; public inject: Server['inject']; public pluginSpecs: any[]; + public uiBundles: any; constructor( settings: Record, diff --git a/src/legacy/ui/ui_render/ui_render_mixin.js b/src/legacy/ui/ui_render/ui_render_mixin.js index fa28b5c8d698c..f8e52180abd27 100644 --- a/src/legacy/ui/ui_render/ui_render_mixin.js +++ b/src/legacy/ui/ui_render/ui_render_mixin.js @@ -19,7 +19,7 @@ import { createHash } from 'crypto'; import Boom from 'boom'; -import { resolve } from 'path'; +import Path from 'path'; import { i18n } from '@kbn/i18n'; import * as UiSharedDeps from '@kbn/ui-shared-deps'; import { AppBootstrap } from './bootstrap'; @@ -39,7 +39,7 @@ import { DllCompiler } from '../../../optimize/dynamic_dll_plugin'; */ export function uiRenderMixin(kbnServer, server, config) { // render all views from ./views - server.setupViews(resolve(__dirname, 'views')); + server.setupViews(Path.resolve(__dirname, 'views')); const translationsCache = { translations: null, hash: null }; server.route({ @@ -106,15 +106,15 @@ export function uiRenderMixin(kbnServer, server, config) { const styleSheetPaths = [ ...(isCore ? [] : dllStyleChunks), - `${basePath}/bundles/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, ...(darkMode ? [ - `${basePath}/bundles/kbn-ui-shared-deps/${UiSharedDeps.darkCssDistFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.darkCssDistFilename}`, `${basePath}/node_modules/@kbn/ui-framework/dist/kui_dark.css`, `${regularBundlePath}/dark_theme.style.css`, ] : [ - `${basePath}/bundles/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, `${basePath}/node_modules/@kbn/ui-framework/dist/kui_light.css`, `${regularBundlePath}/light_theme.style.css`, ]), @@ -136,6 +136,7 @@ export function uiRenderMixin(kbnServer, server, config) { ]), ]; const uiPluginIds = [...kbnServer.newPlatform.__internals.uiPlugins.public.keys()]; + const buildHash = server.newPlatform.env.packageInfo.buildNum; const jsDependencyPaths = [ ...UiSharedDeps.jsDepFilenames.map( @@ -149,16 +150,15 @@ export function uiRenderMixin(kbnServer, server, config) { ...dllJsChunks, `${regularBundlePath}/commons.bundle.js`, ]), - `${regularBundlePath}/plugin/kibanaUtils/kibanaUtils.plugin.js`, - `${regularBundlePath}/plugin/esUiShared/esUiShared.plugin.js`, - `${regularBundlePath}/plugin/kibanaReact/kibanaReact.plugin.js`, - ...uiPluginIds.map( - pluginId => `${regularBundlePath}/plugin/${pluginId}/${pluginId}.plugin.js` + + ...['kibanaUtils', 'esUiShared', 'kibanaReact', ...uiPluginIds].map( + pluginId => `${regularBundlePath}/${buildHash}/plugin/${pluginId}/${pluginId}.plugin.js` ), ]; // These paths should align with the bundle routes configured in // src/optimize/bundles_route/bundles_route.js + // TODO check if it still required const publicPathMap = JSON.stringify({ core: `${regularBundlePath}/core/`, 'kbn-ui-shared-deps': `${regularBundlePath}/kbn-ui-shared-deps/`, @@ -175,7 +175,7 @@ export function uiRenderMixin(kbnServer, server, config) { styleSheetPaths, publicPathMap, entryBundlePath: isCore - ? `${regularBundlePath}/core/core.entry.js` + ? `${regularBundlePath}/${buildHash}/core/core.entry.js` : `${regularBundlePath}/${app.getId()}.bundle.js`, }, }); diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 43b98a51f436e..8b95919661253 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -45,12 +45,16 @@ export function createBundlesRoute({ basePublicPath, builtCssPath, npUiPluginPublicDirs = [], + buildHash, + isDist = false, }: { regularBundlesPath: string; dllBundlesPath: string; basePublicPath: string; builtCssPath: string; npUiPluginPublicDirs?: any[]; + buildHash: string; + isDist?: boolean; }) { // rather than calculate the fileHash on every request, we // provide a cache object to `resolveDynamicAssetResponse()` that @@ -86,6 +90,19 @@ export function createBundlesRoute({ fileHashCache, replacePublicPath: false, }), + // for plugin.js chunk only + ...npUiPluginPublicDirs.map(({ id, path }) => + buildRouteForBundles({ + publicPath: `${basePublicPath}/bundles/plugin/${id}/`, + routePath: `/bundles/${buildHash}/plugin/${id}/`, + bundlesPath: path, + fileHashCache, + isImmutable: isDist, + replacePublicPath: false, + }) + ), + // for async chunks that loaded by webpack. + // we can configure it as publicPath in webpack though ...npUiPluginPublicDirs.map(({ id, path }) => buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/plugin/${id}/`, @@ -97,9 +114,10 @@ export function createBundlesRoute({ ), buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/core/`, - routePath: `/bundles/core/`, + routePath: `/bundles/${buildHash}/core/`, bundlesPath: fromRoot(join('src', 'core', 'target', 'public')), fileHashCache, + isImmutable: isDist, replacePublicPath: false, }), buildRouteForBundles({ @@ -128,12 +146,14 @@ function buildRouteForBundles({ routePath, bundlesPath, fileHashCache, + isImmutable = false, replacePublicPath = true, }: { publicPath: string; routePath: string; bundlesPath: string; fileHashCache: LruCache; + isImmutable?: boolean; replacePublicPath?: boolean; }) { return { @@ -156,6 +176,7 @@ function buildRouteForBundles({ bundlesPath, fileHashCache, publicPath, + isImmutable, replacePublicPath, }); }, diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index b61ea1225f8e7..d14eaa3febf77 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -56,16 +56,23 @@ import { replacePlaceholder } from '../public_path_placeholder'; * @property {string} options.publicPath * @property {LruCache} options.fileHashCache */ -export async function createDynamicAssetResponse(options: { +export async function createDynamicAssetResponse({ + request, + h, + bundlesPath, + publicPath, + fileHashCache, + isImmutable, + replacePublicPath, +}: { request: Hapi.Request; h: Hapi.ResponseToolkit; bundlesPath: string; publicPath: string; fileHashCache: LruCache; - replacePublicPath: any; + isImmutable: boolean; + replacePublicPath: boolean; }) { - const { request, h, bundlesPath, publicPath, fileHashCache, replacePublicPath } = options; - let fd: number | undefined; try { const path = resolve(bundlesPath, request.params.path); @@ -90,6 +97,15 @@ export async function createDynamicAssetResponse(options: { }); fd = undefined; // read stream is now responsible for fd + if (isImmutable) { + return h + .response(read) + .takeover() + .code(200) + .header('cache-control', 'max-age=31536000') + .type(request.server.mime.path(path).type); + } + const content = replacePublicPath ? replacePlaceholder(read, publicPath) : read; const etag = replacePublicPath ? `${hash}-${publicPath}` : hash; diff --git a/src/optimize/bundles_route/index.js b/src/optimize/bundles_route/index.ts similarity index 100% rename from src/optimize/bundles_route/index.js rename to src/optimize/bundles_route/index.ts diff --git a/src/optimize/bundles_route/proxy_bundles_route.js b/src/optimize/bundles_route/proxy_bundles_route.ts similarity index 87% rename from src/optimize/bundles_route/proxy_bundles_route.js rename to src/optimize/bundles_route/proxy_bundles_route.ts index fff0ec444d95b..8e682d09b59f0 100644 --- a/src/optimize/bundles_route/proxy_bundles_route.js +++ b/src/optimize/bundles_route/proxy_bundles_route.ts @@ -17,7 +17,7 @@ * under the License. */ -export function createProxyBundlesRoute({ host, port }) { +export function createProxyBundlesRoute({ host, port }: { host: string; port: string }) { return [ buildProxyRouteForBundles('/bundles/', host, port), buildProxyRouteForBundles('/built_assets/dlls/', host, port), @@ -25,7 +25,7 @@ export function createProxyBundlesRoute({ host, port }) { ]; } -function buildProxyRouteForBundles(routePath, host, port) { +function buildProxyRouteForBundles(routePath: string, host: string, port: string) { return { path: `${routePath}{path*}`, method: 'GET', diff --git a/src/optimize/index.js b/src/optimize/index.ts similarity index 85% rename from src/optimize/index.js rename to src/optimize/index.ts index b7b9f7712358a..54282b990d0c6 100644 --- a/src/optimize/index.js +++ b/src/optimize/index.ts @@ -17,13 +17,18 @@ * under the License. */ +import Hapi from 'hapi'; +// @ts-ignore import FsOptimizer from './fs_optimizer'; import { createBundlesRoute } from './bundles_route'; +// @ts-ignore import { DllCompiler } from './dynamic_dll_plugin'; import { fromRoot } from '../core/server/utils'; import { getNpUiPluginPublicDirs } from './np_ui_plugin_public_dirs'; +import KbnServer, { KibanaConfig } from '../legacy/server/kbn_server'; -export default async (kbnServer, server, config) => { +// eslint-disable-next-line import/no-default-export +export default async (kbnServer: KbnServer, server: Hapi.Server, config: KibanaConfig) => { if (!config.get('optimize.enabled')) return; // the watch optimizer sets up two threads, one is the server listening @@ -36,6 +41,7 @@ export default async (kbnServer, server, config) => { // to prevent complete rebuilds of the optimize content. const watch = config.get('optimize.watch'); if (watch) { + // eslint-disable-next-line @typescript-eslint/no-var-requires return await kbnServer.mixin(require('./watch/watch')); } @@ -47,6 +53,8 @@ export default async (kbnServer, server, config) => { basePublicPath: config.get('server.basePath'), builtCssPath: fromRoot('built_assets/css'), npUiPluginPublicDirs: getNpUiPluginPublicDirs(kbnServer), + buildHash: kbnServer.newPlatform.env.packageInfo.buildNum.toString(), + isDist: kbnServer.newPlatform.env.packageInfo.dist, }) ); @@ -65,7 +73,7 @@ export default async (kbnServer, server, config) => { // only require the FsOptimizer when we need to const optimizer = new FsOptimizer({ - logWithMetadata: (tags, message, metadata) => server.logWithMetadata(tags, message, metadata), + logWithMetadata: server.logWithMetadata, uiBundles, profile: config.get('optimize.profile'), sourceMaps: config.get('optimize.sourceMaps'), diff --git a/src/optimize/watch/optmzr_role.js b/src/optimize/watch/optmzr_role.js index a31ef7229e5da..1f6107996277c 100644 --- a/src/optimize/watch/optmzr_role.js +++ b/src/optimize/watch/optmzr_role.js @@ -49,7 +49,8 @@ export default async (kbnServer, kibanaHapiServer, config) => { config.get('optimize.watchPort'), config.get('server.basePath'), watchOptimizer, - getNpUiPluginPublicDirs(kbnServer) + getNpUiPluginPublicDirs(kbnServer), + kbnServer.newPlatform.env.packageInfo.buildNum.toString() ); watchOptimizer.status$.subscribe({ diff --git a/src/optimize/watch/watch_optimizer.js b/src/optimize/watch/watch_optimizer.js index 6c20f21c7768e..cdff57a00c2e0 100644 --- a/src/optimize/watch/watch_optimizer.js +++ b/src/optimize/watch/watch_optimizer.js @@ -106,7 +106,7 @@ export default class WatchOptimizer extends BaseOptimizer { }); } - bindToServer(server, basePath, npUiPluginPublicDirs) { + bindToServer(server, basePath, npUiPluginPublicDirs, buildHash) { // pause all requests received while the compiler is running // and continue once an outcome is reached (aborting the request // with an error if it was a failure). @@ -118,6 +118,7 @@ export default class WatchOptimizer extends BaseOptimizer { server.route( createBundlesRoute({ npUiPluginPublicDirs: npUiPluginPublicDirs, + buildHash, regularBundlesPath: this.compiler.outputPath, dllBundlesPath: DllCompiler.getRawDllConfig().outputPath, basePublicPath: basePath, diff --git a/src/optimize/watch/watch_server.js b/src/optimize/watch/watch_server.js index 74a96dc8aea6e..81e04a5b83956 100644 --- a/src/optimize/watch/watch_server.js +++ b/src/optimize/watch/watch_server.js @@ -21,10 +21,11 @@ import { Server } from 'hapi'; import { registerHapiPlugins } from '../../legacy/server/http/register_hapi_plugins'; export default class WatchServer { - constructor(host, port, basePath, optimizer, npUiPluginPublicDirs) { + constructor(host, port, basePath, optimizer, npUiPluginPublicDirs, buildHash) { this.basePath = basePath; this.optimizer = optimizer; this.npUiPluginPublicDirs = npUiPluginPublicDirs; + this.buildHash = buildHash; this.server = new Server({ host: host, port: port, @@ -35,7 +36,12 @@ export default class WatchServer { async init() { await this.optimizer.init(); - this.optimizer.bindToServer(this.server, this.basePath, this.npUiPluginPublicDirs); + this.optimizer.bindToServer( + this.server, + this.basePath, + this.npUiPluginPublicDirs, + this.buildHash + ); await this.server.start(); } } From f2c8d5102997d27b5e888a1fc7c603cee9c546d7 Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Mon, 27 Apr 2020 12:15:39 +0200 Subject: [PATCH 04/19] add tests for caching --- .../bundles_route/__tests__/bundles_route.js | 106 +++++++++++++++++- .../bundles_route/dynamic_asset_response.ts | 10 +- 2 files changed, 111 insertions(+), 5 deletions(-) diff --git a/src/optimize/bundles_route/__tests__/bundles_route.js b/src/optimize/bundles_route/__tests__/bundles_route.js index 0b2aeda11fb0e..a99951b391aa3 100644 --- a/src/optimize/bundles_route/__tests__/bundles_route.js +++ b/src/optimize/bundles_route/__tests__/bundles_route.js @@ -32,6 +32,8 @@ import { PUBLIC_PATH_PLACEHOLDER } from '../../public_path_placeholder'; const chance = new Chance(); const outputFixture = resolve(__dirname, './fixtures/output'); +const pluginNoPlaceholderFixture = resolve(__dirname, './fixtures/plugin/no_placeholder'); +const pluginPlaceholderFixture = resolve(__dirname, './fixtures/plugin/placeholder'); const randomWordsCache = new Set(); const uniqueRandomWord = () => { @@ -58,6 +60,9 @@ describe('optimizer/bundle route', () => { dllBundlesPath = outputFixture, basePublicPath = '', builtCssPath = outputFixture, + npUiPluginPublicDirs = [], + buildHash = '1234', + isDist = false, } = options; const server = new Hapi.Server(); @@ -69,6 +74,9 @@ describe('optimizer/bundle route', () => { dllBundlesPath, basePublicPath, builtCssPath, + npUiPluginPublicDirs, + buildHash, + isDist, }) ); @@ -321,7 +329,7 @@ describe('optimizer/bundle route', () => { expect(resp2.statusCode).to.be(200); }); - it('is unique per basePublicPath although content is the same', async () => { + it('is unique per basePublicPath although content is the same (by default)', async () => { const basePublicPath1 = `/${uniqueRandomWord()}`; const basePublicPath2 = `/${uniqueRandomWord()}`; @@ -366,4 +374,100 @@ describe('optimizer/bundle route', () => { expect(resp2.result).to.have.length(0); }); }); + + describe('kibana platform assets', () => { + describe('caching', () => { + describe('for non-distributable mode', () => { + it('uses "etag" header to invalidate cache', async () => { + const basePublicPath = `/${uniqueRandomWord()}`; + + const npUiPluginPublicDirs = [ + { + id: 'no_placeholder', + path: pluginNoPlaceholderFixture, + }, + ]; + const responce = await createServer({ basePublicPath, npUiPluginPublicDirs }).inject({ + url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + }); + + expect(responce.statusCode).to.be(200); + + expect(responce.headers.etag).to.be.a('string'); + expect(responce.headers['cache-control']).to.be('must-revalidate'); + }); + + it('creates the same "etag" header for the same content', async () => { + const basePublicPath1 = `/${uniqueRandomWord()}`; + const basePublicPath2 = `/${uniqueRandomWord()}`; + + const npUiPluginPublicDirs = [ + { + id: 'no_placeholder', + path: pluginNoPlaceholderFixture, + }, + ]; + const [resp1, resp2] = await Promise.all([ + createServer({ basePublicPath: basePublicPath1, npUiPluginPublicDirs }).inject({ + url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + }), + createServer({ basePublicPath: basePublicPath2, npUiPluginPublicDirs }).inject({ + url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + }), + ]); + + expect(resp1.statusCode).to.be(200); + expect(resp2.statusCode).to.be(200); + + expect(resp1.rawPayload).to.eql(resp2.rawPayload); + + expect(resp1.headers.etag).to.be.a('string'); + expect(resp2.headers.etag).to.be.a('string'); + expect(resp1.headers.etag).to.eql(resp2.headers.etag); + }); + + it('does not replace placeholder', async () => { + const basePublicPath = `/${uniqueRandomWord()}`; + + const npUiPluginPublicDirs = [ + { + id: 'placeholder', + path: pluginPlaceholderFixture, + }, + ]; + const response = await createServer({ basePublicPath, npUiPluginPublicDirs }).inject({ + url: '/bundles/1234/plugin/placeholder/placeholder.plugin.js', + }); + + expect(response.statusCode).to.be(200); + expect(response.result.includes('__REPLACE_WITH_PUBLIC_PATH__/FOO')).to.be(true); + }); + }); + + describe('for distributable mode', () => { + it('commands to cache assets for each release for a year', async () => { + const basePublicPath = `/${uniqueRandomWord()}`; + + const npUiPluginPublicDirs = [ + { + id: 'no_placeholder', + path: pluginNoPlaceholderFixture, + }, + ]; + const responce = await createServer({ + basePublicPath, + npUiPluginPublicDirs, + isDist: true, + }).inject({ + url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + }); + + expect(responce.statusCode).to.be(200); + + expect(responce.headers.etag).to.be(undefined); + expect(responce.headers['cache-control']).to.be('max-age=31536000'); + }); + }); + }); + }); }); diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index d14eaa3febf77..3b36f29be0cf8 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -87,17 +87,15 @@ export async function createDynamicAssetResponse({ // the file 2 or 3 times per request it seems logical fd = await fcb(cb => open(path, 'r', cb)); - const stat = await fcb(cb => fstat(fd!, cb)); - const hash = await getFileHash(fileHashCache, path, stat, fd); - const read = createReadStream(null as any, { fd, start: 0, autoClose: true, }); - fd = undefined; // read stream is now responsible for fd if (isImmutable) { + fd = undefined; // read stream is now responsible for fd + return h .response(read) .takeover() @@ -106,6 +104,10 @@ export async function createDynamicAssetResponse({ .type(request.server.mime.path(path).type); } + const stat = await fcb(cb => fstat(fd!, cb)); + const hash = await getFileHash(fileHashCache, path, stat, fd); + fd = undefined; // read stream is now responsible for fd + const content = replacePublicPath ? replacePlaceholder(read, publicPath) : read; const etag = replacePublicPath ? `${hash}-${publicPath}` : hash; From 9f46398e97a544163cce34f855f1a69263aa4cd2 Mon Sep 17 00:00:00 2001 From: restrry Date: Mon, 27 Apr 2020 14:59:18 +0200 Subject: [PATCH 05/19] fix tests --- src/core/public/plugins/plugin.test.mocks.ts | 6 +-- src/core/public/plugins/plugin.test.ts | 32 ++++++------ src/core/public/plugins/plugin.ts | 44 +++------------- src/core/public/plugins/plugin_reader.ts | 50 +++++++++++++++++++ .../plugins/plugins_service.test.mocks.ts | 8 ++- .../public/plugins/plugins_service.test.ts | 16 +----- 6 files changed, 83 insertions(+), 73 deletions(-) create mode 100644 src/core/public/plugins/plugin_reader.ts diff --git a/src/core/public/plugins/plugin.test.mocks.ts b/src/core/public/plugins/plugin.test.mocks.ts index b877847aaa90e..422442c9ca4d2 100644 --- a/src/core/public/plugins/plugin.test.mocks.ts +++ b/src/core/public/plugins/plugin.test.mocks.ts @@ -24,8 +24,8 @@ export const mockPlugin = { }; export const mockInitializer = jest.fn(() => mockPlugin); -export const mockPluginLoader = jest.fn().mockResolvedValue(mockInitializer); +export const mockPluginReader = jest.fn(() => mockInitializer); -jest.mock('./plugin_loader', () => ({ - loadPluginBundle: mockPluginLoader, +jest.mock('./plugin_reader', () => ({ + read: mockPluginReader, })); diff --git a/src/core/public/plugins/plugin.test.ts b/src/core/public/plugins/plugin.test.ts index 4ed2106d542d5..8fe745db9554d 100644 --- a/src/core/public/plugins/plugin.test.ts +++ b/src/core/public/plugins/plugin.test.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { mockInitializer, mockPlugin, mockPluginLoader } from './plugin.test.mocks'; +import { mockInitializer, mockPlugin, mockPluginReader } from './plugin.test.mocks'; import { DiscoveredPlugin } from '../../server'; import { coreMock } from '../mocks'; @@ -40,7 +40,7 @@ const opaqueId = Symbol(); const initializerContext = coreMock.createPluginInitializerContext(); beforeEach(() => { - mockPluginLoader.mockClear(); + mockPluginReader.mockClear(); mockPlugin.setup.mockClear(); mockPlugin.start.mockClear(); mockPlugin.stop.mockClear(); @@ -48,12 +48,6 @@ beforeEach(() => { }); describe('PluginWrapper', () => { - test('`setup` fails if load is not called first', async () => { - await expect(plugin.setup({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot( - `"Plugin \\"plugin-a\\" can't be setup since its bundle isn't loaded."` - ); - }); - test('`setup` fails if plugin.setup is not a function', async () => { mockInitializer.mockReturnValueOnce({ start: jest.fn() } as any); await expect(plugin.setup({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot( @@ -102,19 +96,21 @@ describe('PluginWrapper', () => { }; let startDependenciesResolved = false; - mockPluginLoader.mockResolvedValueOnce(() => ({ - setup: jest.fn(), - start: async () => { - // Add small delay to ensure startDependencies is not resolved until after the plugin instance's start resolves. - await new Promise(resolve => setTimeout(resolve, 10)); - expect(startDependenciesResolved).toBe(false); - return pluginStartContract; - }, - })); + mockPluginReader.mockReturnValueOnce( + jest.fn(() => ({ + setup: jest.fn(), + start: jest.fn(async () => { + // Add small delay to ensure startDependencies is not resolved until after the plugin instance's start resolves. + await new Promise(resolve => setTimeout(resolve, 10)); + expect(startDependenciesResolved).toBe(false); + return pluginStartContract; + }), + stop: jest.fn(), + })) + ); await plugin.setup({} as any, {} as any); const context = { any: 'thing' } as any; const deps = { otherDep: 'value' }; - // Add promise callback prior to calling `start` to ensure calls in `setup` will not resolve before `start` is // called. const startDependenciesCheck = plugin.startDependencies.then(res => { diff --git a/src/core/public/plugins/plugin.ts b/src/core/public/plugins/plugin.ts index 8957b7f4f1508..591165fcd2839 100644 --- a/src/core/public/plugins/plugin.ts +++ b/src/core/public/plugins/plugin.ts @@ -21,24 +21,9 @@ import { Subject } from 'rxjs'; import { first } from 'rxjs/operators'; import { DiscoveredPlugin, PluginOpaqueId } from '../../server'; import { PluginInitializerContext } from './plugin_context'; +import { read } from './plugin_reader'; import { CoreStart, CoreSetup } from '..'; -/** - * Unknown variant for internal use only for when plugins are not known. - * @internal - */ -export type UnknownPluginInitializer = PluginInitializer>; - -/** - * Custom window type for loading bundles. Do not extend global Window to avoid leaking these types. - * @internal - */ -export interface CoreWindow { - __kbnBundles__: { - [pluginBundleName: string]: { plugin: UnknownPluginInitializer } | undefined; - }; -} - /** * The interface that should be returned by a `PluginInitializer`. * @@ -100,25 +85,6 @@ export class PluginWrapper< this.optionalPlugins = discoveredPlugin.optionalPlugins; } - /** - * Reads the plugin's bundle declared in the global context. - */ - private read() { - const coreWindow = (window as unknown) as CoreWindow; - const exportId = `plugin/${this.name}`; - const pluginExport = coreWindow.__kbnBundles__[exportId]; - if (typeof pluginExport!.plugin !== 'function') { - throw new Error(`Definition of plugin "${this.name}" should be a function.`); - } else { - return pluginExport!.plugin as PluginInitializer< - TSetup, - TStart, - TPluginsSetup, - TPluginsStart - >; - } - } - /** * Instantiates plugin and calls `setup` function exposed by the plugin initializer. * @param setupContext Context that consists of various core services tailored specifically @@ -167,7 +133,13 @@ export class PluginWrapper< } private async createPluginInstance() { - const initializer = this.read(); + const initializer = read(this.name) as PluginInitializer< + TSetup, + TStart, + TPluginsSetup, + TPluginsStart + >; + const instance = initializer(this.initializerContext); if (typeof instance.setup !== 'function') { diff --git a/src/core/public/plugins/plugin_reader.ts b/src/core/public/plugins/plugin_reader.ts new file mode 100644 index 0000000000000..55271c6e3af6a --- /dev/null +++ b/src/core/public/plugins/plugin_reader.ts @@ -0,0 +1,50 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you 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. + */ + +import { PluginInitializer } from './plugin'; + +/** + * Unknown variant for internal use only for when plugins are not known. + * @internal + */ +export type UnknownPluginInitializer = PluginInitializer>; + +/** + * Custom window type for loading bundles. Do not extend global Window to avoid leaking these types. + * @internal + */ +export interface CoreWindow { + __kbnBundles__: { + [pluginBundleName: string]: { plugin: UnknownPluginInitializer } | undefined; + }; +} + +/** + * Reads the plugin's bundle declared in the global context. + */ +export function read(name: string) { + const coreWindow = (window as unknown) as CoreWindow; + const exportId = `plugin/${name}`; + const pluginExport = coreWindow.__kbnBundles__[exportId]; + if (typeof pluginExport!.plugin !== 'function') { + throw new Error(`Definition of plugin "${name}" should be a function.`); + } else { + return pluginExport!.plugin; + } +} diff --git a/src/core/public/plugins/plugins_service.test.mocks.ts b/src/core/public/plugins/plugins_service.test.mocks.ts index 46879c8133366..85b84e561056a 100644 --- a/src/core/public/plugins/plugins_service.test.mocks.ts +++ b/src/core/public/plugins/plugins_service.test.mocks.ts @@ -25,4 +25,10 @@ export type MockedPluginInitializer = jest.Mock = jest.fn().mockRejectedValue(new Error('No provider specified')); +> = jest.fn().mockImplementation(() => () => { + throw new Error('No provider specified'); +}); + +jest.mock('./plugin_reader', () => ({ + read: mockPluginInitializerProvider, +})); diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index d8b627747e4df..6d71844bc19c8 100644 --- a/src/core/public/plugins/plugins_service.test.ts +++ b/src/core/public/plugins/plugins_service.test.ts @@ -31,6 +31,7 @@ import { PluginsServiceStartDeps, PluginsServiceSetupDeps, } from './plugins_service'; + import { InjectedPluginMetadata } from '../injected_metadata'; import { notificationServiceMock } from '../notifications/notifications_service.mock'; import { applicationServiceMock } from '../application/application_service.mock'; @@ -151,10 +152,6 @@ describe('PluginsService', () => { ] as unknown) as [[PluginName, any]]); }); - afterEach(() => { - // mockLoadPluginBundle.mockClear(); - }); - describe('#getOpaqueIds()', () => { it('returns dependency tree of symbols', () => { const pluginsService = new PluginsService(mockCoreContext, plugins); @@ -173,16 +170,6 @@ describe('PluginsService', () => { }); describe('#setup()', () => { - it('fails if any bundle cannot be loaded', async () => { - // TODO - // mockLoadPluginBundle.mockRejectedValueOnce(new Error('Could not load bundle')); - - const pluginsService = new PluginsService(mockCoreContext, plugins); - await expect(pluginsService.setup(mockSetupDeps)).rejects.toThrowErrorMatchingInlineSnapshot( - `"Could not load bundle"` - ); - }); - it('fails if any plugin instance does not have a setup function', async () => { mockPluginInitializers.set('pluginA', (() => ({})) as any); const pluginsService = new PluginsService(mockCoreContext, plugins); @@ -283,7 +270,6 @@ describe('PluginsService', () => { const pluginsService = new PluginsService(mockCoreContext, plugins); const promise = pluginsService.setup(mockSetupDeps); - jest.runAllTimers(); // load plugin bundles await flushPromises(); jest.runAllTimers(); // setup plugins From 5e5c1450015d2b64c579b95155bd89869b6fce15 Mon Sep 17 00:00:00 2001 From: restrry Date: Mon, 27 Apr 2020 15:17:27 +0200 Subject: [PATCH 06/19] remove the last TODO. url should be inlined with assetss server --- src/legacy/ui/ui_render/ui_render_mixin.js | 10 ++++++---- src/optimize/bundles_route/bundles_route.ts | 1 - 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/legacy/ui/ui_render/ui_render_mixin.js b/src/legacy/ui/ui_render/ui_render_mixin.js index f8e52180abd27..e146adcb1319e 100644 --- a/src/legacy/ui/ui_render/ui_render_mixin.js +++ b/src/legacy/ui/ui_render/ui_render_mixin.js @@ -157,13 +157,15 @@ export function uiRenderMixin(kbnServer, server, config) { ]; // These paths should align with the bundle routes configured in - // src/optimize/bundles_route/bundles_route.js - // TODO check if it still required + // src/optimize/bundles_route/bundles_route.ts const publicPathMap = JSON.stringify({ - core: `${regularBundlePath}/core/`, + core: `${regularBundlePath}/${buildHash}/core/`, 'kbn-ui-shared-deps': `${regularBundlePath}/kbn-ui-shared-deps/`, ...uiPluginIds.reduce( - (acc, pluginId) => ({ ...acc, [pluginId]: `${regularBundlePath}/plugin/${pluginId}/` }), + (acc, pluginId) => ({ + ...acc, + [pluginId]: `${regularBundlePath}/${buildHash}/plugin/${pluginId}/`, + }), {} ), }); diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 8b95919661253..0f430b0de1448 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -102,7 +102,6 @@ export function createBundlesRoute({ }) ), // for async chunks that loaded by webpack. - // we can configure it as publicPath in webpack though ...npUiPluginPublicDirs.map(({ id, path }) => buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/plugin/${id}/`, From 03b1584f6aaa329996a4a10f8ddd5a160737f7b8 Mon Sep 17 00:00:00 2001 From: restrry Date: Mon, 27 Apr 2020 16:06:10 +0200 Subject: [PATCH 07/19] this logic handled by publicPathMap on the client --- src/optimize/bundles_route/bundles_route.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 0f430b0de1448..7299128fec527 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -90,7 +90,6 @@ export function createBundlesRoute({ fileHashCache, replacePublicPath: false, }), - // for plugin.js chunk only ...npUiPluginPublicDirs.map(({ id, path }) => buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/plugin/${id}/`, @@ -101,16 +100,6 @@ export function createBundlesRoute({ replacePublicPath: false, }) ), - // for async chunks that loaded by webpack. - ...npUiPluginPublicDirs.map(({ id, path }) => - buildRouteForBundles({ - publicPath: `${basePublicPath}/bundles/plugin/${id}/`, - routePath: `/bundles/plugin/${id}/`, - bundlesPath: path, - fileHashCache, - replacePublicPath: false, - }) - ), buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/core/`, routePath: `/bundles/${buildHash}/core/`, From 5082c6e21b9e8cc6d6dc5fd3831a310264e7cd84 Mon Sep 17 00:00:00 2001 From: restrry Date: Mon, 27 Apr 2020 16:20:25 +0200 Subject: [PATCH 08/19] cache kbn-shared-deps as well --- src/legacy/ui/ui_render/ui_render_mixin.js | 14 +++++++------- src/optimize/bundles_route/bundles_route.ts | 3 ++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/legacy/ui/ui_render/ui_render_mixin.js b/src/legacy/ui/ui_render/ui_render_mixin.js index e146adcb1319e..aef4707ced865 100644 --- a/src/legacy/ui/ui_render/ui_render_mixin.js +++ b/src/legacy/ui/ui_render/ui_render_mixin.js @@ -103,18 +103,19 @@ export function uiRenderMixin(kbnServer, server, config) { const dllJsChunks = DllCompiler.getRawDllConfig().chunks.map( chunk => `${dllBundlePath}/vendors${chunk}.bundle.dll.js` ); + const buildHash = server.newPlatform.env.packageInfo.buildNum; const styleSheetPaths = [ ...(isCore ? [] : dllStyleChunks), - `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, + `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, ...(darkMode ? [ - `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.darkCssDistFilename}`, + `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.darkCssDistFilename}`, `${basePath}/node_modules/@kbn/ui-framework/dist/kui_dark.css`, `${regularBundlePath}/dark_theme.style.css`, ] : [ - `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, + `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, `${basePath}/node_modules/@kbn/ui-framework/dist/kui_light.css`, `${regularBundlePath}/light_theme.style.css`, ]), @@ -136,13 +137,12 @@ export function uiRenderMixin(kbnServer, server, config) { ]), ]; const uiPluginIds = [...kbnServer.newPlatform.__internals.uiPlugins.public.keys()]; - const buildHash = server.newPlatform.env.packageInfo.buildNum; const jsDependencyPaths = [ ...UiSharedDeps.jsDepFilenames.map( - filename => `${regularBundlePath}/kbn-ui-shared-deps/${filename}` + filename => `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${filename}` ), - `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, + `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, ...(isCore ? [] : [ @@ -160,7 +160,7 @@ export function uiRenderMixin(kbnServer, server, config) { // src/optimize/bundles_route/bundles_route.ts const publicPathMap = JSON.stringify({ core: `${regularBundlePath}/${buildHash}/core/`, - 'kbn-ui-shared-deps': `${regularBundlePath}/kbn-ui-shared-deps/`, + 'kbn-ui-shared-deps': `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/`, ...uiPluginIds.reduce( (acc, pluginId) => ({ ...acc, diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 7299128fec527..cd3efe547849a 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -85,9 +85,10 @@ export function createBundlesRoute({ return [ buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/kbn-ui-shared-deps/`, - routePath: '/bundles/kbn-ui-shared-deps/', + routePath: `/bundles/${buildHash}/kbn-ui-shared-deps/`, bundlesPath: UiSharedDeps.distDir, fileHashCache, + isImmutable: isDist, replacePublicPath: false, }), ...npUiPluginPublicDirs.map(({ id, path }) => From c0332e139a1dd70d088d4ef4dc72117c22db4831 Mon Sep 17 00:00:00 2001 From: restrry Date: Mon, 27 Apr 2020 19:26:46 +0200 Subject: [PATCH 09/19] attempt to fix karma tests --- tasks/config/karma.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tasks/config/karma.js b/tasks/config/karma.js index 1ec7c831b4864..5b3259eb77d14 100644 --- a/tasks/config/karma.js +++ b/tasks/config/karma.js @@ -57,9 +57,10 @@ module.exports = function(grunt) { 'http://localhost:5610/test_bundle/karma/globals.js', ...UiSharedDeps.jsDepFilenames.map( - chunkFilename => `http://localhost:5610/bundles/kbn-ui-shared-deps/${chunkFilename}` + chunkFilename => + `http://localhost:5610/bundles/9007199254740991/kbn-ui-shared-deps/${chunkFilename}` ), - `http://localhost:5610/bundles/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, + `http://localhost:5610/bundles/9007199254740991/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, 'http://localhost:5610/built_assets/dlls/vendors_runtime.bundle.dll.js', ...DllCompiler.getRawDllConfig().chunks.map( @@ -70,7 +71,7 @@ module.exports = function(grunt) { ? `http://localhost:5610/bundles/tests.bundle.js` : `http://localhost:5610/bundles/tests.bundle.js?shards=${TOTAL_CI_SHARDS}&shard_num=${shardNum}`, - `http://localhost:5610/bundles/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, + `http://localhost:5610/bundles/9007199254740991/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, // this causes tilemap tests to fail, probably because the eui styles haven't been // included in the karma harness a long some time, if ever // `http://localhost:5610/bundles/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, From c66b36284c6d88ff83cce26ad7527705c9de7789 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 27 Apr 2020 13:05:59 -0700 Subject: [PATCH 10/19] always run file through replace stream --- src/optimize/bundles_route/bundles_route.ts | 13 +++----- .../bundles_route/dynamic_asset_response.ts | 33 +++++++------------ 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index cd3efe547849a..2fab08b9a0354 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -89,7 +89,6 @@ export function createBundlesRoute({ bundlesPath: UiSharedDeps.distDir, fileHashCache, isImmutable: isDist, - replacePublicPath: false, }), ...npUiPluginPublicDirs.map(({ id, path }) => buildRouteForBundles({ @@ -98,7 +97,6 @@ export function createBundlesRoute({ bundlesPath: path, fileHashCache, isImmutable: isDist, - replacePublicPath: false, }) ), buildRouteForBundles({ @@ -107,25 +105,27 @@ export function createBundlesRoute({ bundlesPath: fromRoot(join('src', 'core', 'target', 'public')), fileHashCache, isImmutable: isDist, - replacePublicPath: false, }), buildRouteForBundles({ publicPath: `${basePublicPath}/bundles/`, routePath: '/bundles/', bundlesPath: regularBundlesPath, fileHashCache, + isImmutable: isDist, }), buildRouteForBundles({ publicPath: `${basePublicPath}/built_assets/dlls/`, routePath: '/built_assets/dlls/', bundlesPath: dllBundlesPath, fileHashCache, + isImmutable: isDist, }), buildRouteForBundles({ publicPath: `${basePublicPath}/`, routePath: '/built_assets/css/', bundlesPath: builtCssPath, fileHashCache, + isImmutable: isDist, }), ]; } @@ -135,15 +135,13 @@ function buildRouteForBundles({ routePath, bundlesPath, fileHashCache, - isImmutable = false, - replacePublicPath = true, + isImmutable, }: { publicPath: string; routePath: string; bundlesPath: string; fileHashCache: LruCache; - isImmutable?: boolean; - replacePublicPath?: boolean; + isImmutable: boolean; }) { return { method: 'GET', @@ -166,7 +164,6 @@ function buildRouteForBundles({ fileHashCache, publicPath, isImmutable, - replacePublicPath, }); }, }, diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index 3b36f29be0cf8..70cfb0f531836 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -63,7 +63,6 @@ export async function createDynamicAssetResponse({ publicPath, fileHashCache, isImmutable, - replacePublicPath, }: { request: Hapi.Request; h: Hapi.ResponseToolkit; @@ -71,7 +70,6 @@ export async function createDynamicAssetResponse({ publicPath: string; fileHashCache: LruCache; isImmutable: boolean; - replacePublicPath: boolean; }) { let fd: number | undefined; try { @@ -93,31 +91,24 @@ export async function createDynamicAssetResponse({ autoClose: true, }); - if (isImmutable) { - fd = undefined; // read stream is now responsible for fd - - return h - .response(read) - .takeover() - .code(200) - .header('cache-control', 'max-age=31536000') - .type(request.server.mime.path(path).type); - } - const stat = await fcb(cb => fstat(fd!, cb)); - const hash = await getFileHash(fileHashCache, path, stat, fd); + const hash = isImmutable ? undefined : await getFileHash(fileHashCache, path, stat, fd); fd = undefined; // read stream is now responsible for fd - const content = replacePublicPath ? replacePlaceholder(read, publicPath) : read; - const etag = replacePublicPath ? `${hash}-${publicPath}` : hash; - - return h - .response(content) + const response = h + .response(replacePlaceholder(read, publicPath)) .takeover() .code(200) - .etag(etag) - .header('cache-control', 'must-revalidate') .type(request.server.mime.path(path).type); + + if (isImmutable) { + response.header('cache-control', 'max-age=31536000'); + } else { + response.etag(`${hash}-${publicPath}`); + response.header('cache-control', 'must-revalidate'); + } + + return response; } catch (error) { if (fd) { try { From b1d7133908e02802a82c38c6028cb6fef4ceeac2 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 27 Apr 2020 14:53:49 -0700 Subject: [PATCH 11/19] place buildHash at begining of path, include all static files --- src/legacy/ui/ui_render/ui_render_mixin.js | 30 ++++++++++--------- src/optimize/bundles_route/bundles_route.ts | 22 +++++++------- .../bundles_route/proxy_bundles_route.ts | 16 +++++++--- src/optimize/watch/proxy_role.js | 1 + 4 files changed, 40 insertions(+), 29 deletions(-) diff --git a/src/legacy/ui/ui_render/ui_render_mixin.js b/src/legacy/ui/ui_render/ui_render_mixin.js index aef4707ced865..7cd87ae01e2c8 100644 --- a/src/legacy/ui/ui_render/ui_render_mixin.js +++ b/src/legacy/ui/ui_render/ui_render_mixin.js @@ -94,28 +94,30 @@ export function uiRenderMixin(kbnServer, server, config) { ? await uiSettings.get('theme:darkMode') : false; + const buildHash = server.newPlatform.env.packageInfo.buildNum; const basePath = config.get('server.basePath'); - const regularBundlePath = `${basePath}/bundles`; - const dllBundlePath = `${basePath}/built_assets/dlls`; + + const regularBundlePath = `${basePath}/${buildHash}/bundles`; + const dllBundlePath = `${basePath}/${buildHash}/built_assets/dlls`; + const dllStyleChunks = DllCompiler.getRawDllConfig().chunks.map( chunk => `${dllBundlePath}/vendors${chunk}.style.dll.css` ); const dllJsChunks = DllCompiler.getRawDllConfig().chunks.map( chunk => `${dllBundlePath}/vendors${chunk}.bundle.dll.js` ); - const buildHash = server.newPlatform.env.packageInfo.buildNum; const styleSheetPaths = [ ...(isCore ? [] : dllStyleChunks), - `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, ...(darkMode ? [ - `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.darkCssDistFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.darkCssDistFilename}`, `${basePath}/node_modules/@kbn/ui-framework/dist/kui_dark.css`, `${regularBundlePath}/dark_theme.style.css`, ] : [ - `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, `${basePath}/node_modules/@kbn/ui-framework/dist/kui_light.css`, `${regularBundlePath}/light_theme.style.css`, ]), @@ -130,7 +132,7 @@ export function uiRenderMixin(kbnServer, server, config) { ) .map(path => path.localPath.endsWith('.scss') - ? `${basePath}/built_assets/css/${path.publicPath}` + ? `${basePath}/${buildHash}/built_assets/css/${path.publicPath}` : `${basePath}/${path.publicPath}` ) .reverse(), @@ -140,9 +142,9 @@ export function uiRenderMixin(kbnServer, server, config) { const jsDependencyPaths = [ ...UiSharedDeps.jsDepFilenames.map( - filename => `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${filename}` + filename => `${regularBundlePath}/kbn-ui-shared-deps/${filename}` ), - `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, + `${regularBundlePath}/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, ...(isCore ? [] : [ @@ -152,19 +154,19 @@ export function uiRenderMixin(kbnServer, server, config) { ]), ...['kibanaUtils', 'esUiShared', 'kibanaReact', ...uiPluginIds].map( - pluginId => `${regularBundlePath}/${buildHash}/plugin/${pluginId}/${pluginId}.plugin.js` + pluginId => `${regularBundlePath}/plugin/${pluginId}/${pluginId}.plugin.js` ), ]; // These paths should align with the bundle routes configured in // src/optimize/bundles_route/bundles_route.ts const publicPathMap = JSON.stringify({ - core: `${regularBundlePath}/${buildHash}/core/`, - 'kbn-ui-shared-deps': `${regularBundlePath}/${buildHash}/kbn-ui-shared-deps/`, + core: `${regularBundlePath}/core/`, + 'kbn-ui-shared-deps': `${regularBundlePath}/kbn-ui-shared-deps/`, ...uiPluginIds.reduce( (acc, pluginId) => ({ ...acc, - [pluginId]: `${regularBundlePath}/${buildHash}/plugin/${pluginId}/`, + [pluginId]: `${regularBundlePath}/plugin/${pluginId}/`, }), {} ), @@ -177,7 +179,7 @@ export function uiRenderMixin(kbnServer, server, config) { styleSheetPaths, publicPathMap, entryBundlePath: isCore - ? `${regularBundlePath}/${buildHash}/core/core.entry.js` + ? `${regularBundlePath}/core/core.entry.js` : `${regularBundlePath}/${app.getId()}.bundle.js`, }, }); diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 2fab08b9a0354..61b3dca6cb85f 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -84,45 +84,45 @@ export function createBundlesRoute({ return [ buildRouteForBundles({ - publicPath: `${basePublicPath}/bundles/kbn-ui-shared-deps/`, - routePath: `/bundles/${buildHash}/kbn-ui-shared-deps/`, + publicPath: `${basePublicPath}/${buildHash}/bundles/kbn-ui-shared-deps/`, + routePath: `/${buildHash}/bundles/kbn-ui-shared-deps/`, bundlesPath: UiSharedDeps.distDir, fileHashCache, isImmutable: isDist, }), ...npUiPluginPublicDirs.map(({ id, path }) => buildRouteForBundles({ - publicPath: `${basePublicPath}/bundles/plugin/${id}/`, - routePath: `/bundles/${buildHash}/plugin/${id}/`, + publicPath: `${basePublicPath}/${buildHash}/bundles/plugin/${id}/`, + routePath: `/${buildHash}/bundles/plugin/${id}/`, bundlesPath: path, fileHashCache, isImmutable: isDist, }) ), buildRouteForBundles({ - publicPath: `${basePublicPath}/bundles/core/`, - routePath: `/bundles/${buildHash}/core/`, + publicPath: `${basePublicPath}/${buildHash}/bundles/core/`, + routePath: `/${buildHash}/bundles/core/`, bundlesPath: fromRoot(join('src', 'core', 'target', 'public')), fileHashCache, isImmutable: isDist, }), buildRouteForBundles({ - publicPath: `${basePublicPath}/bundles/`, - routePath: '/bundles/', + publicPath: `${basePublicPath}/${buildHash}/bundles/`, + routePath: `/${buildHash}/bundles/`, bundlesPath: regularBundlesPath, fileHashCache, isImmutable: isDist, }), buildRouteForBundles({ - publicPath: `${basePublicPath}/built_assets/dlls/`, - routePath: '/built_assets/dlls/', + publicPath: `${basePublicPath}/${buildHash}/built_assets/dlls/`, + routePath: `/${buildHash}/built_assets/dlls/`, bundlesPath: dllBundlesPath, fileHashCache, isImmutable: isDist, }), buildRouteForBundles({ publicPath: `${basePublicPath}/`, - routePath: '/built_assets/css/', + routePath: `/${buildHash}/built_assets/css/`, bundlesPath: builtCssPath, fileHashCache, isImmutable: isDist, diff --git a/src/optimize/bundles_route/proxy_bundles_route.ts b/src/optimize/bundles_route/proxy_bundles_route.ts index 8e682d09b59f0..9635f788901d2 100644 --- a/src/optimize/bundles_route/proxy_bundles_route.ts +++ b/src/optimize/bundles_route/proxy_bundles_route.ts @@ -17,11 +17,19 @@ * under the License. */ -export function createProxyBundlesRoute({ host, port }: { host: string; port: string }) { +export function createProxyBundlesRoute({ + host, + port, + buildHash, +}: { + host: string; + port: string; + buildHash: string; +}) { return [ - buildProxyRouteForBundles('/bundles/', host, port), - buildProxyRouteForBundles('/built_assets/dlls/', host, port), - buildProxyRouteForBundles('/built_assets/css/', host, port), + buildProxyRouteForBundles(`/${buildHash}/bundles/`, host, port), + buildProxyRouteForBundles(`/${buildHash}/built_assets/dlls/`, host, port), + buildProxyRouteForBundles(`/${buildHash}/built_assets/css/`, host, port), ]; } diff --git a/src/optimize/watch/proxy_role.js b/src/optimize/watch/proxy_role.js index 6093658ae1a2d..0f6f3b2d4b622 100644 --- a/src/optimize/watch/proxy_role.js +++ b/src/optimize/watch/proxy_role.js @@ -26,6 +26,7 @@ export default (kbnServer, server, config) => { createProxyBundlesRoute({ host: config.get('optimize.watchHost'), port: config.get('optimize.watchPort'), + buildHash: kbnServer.newPlatform.env.packageInfo.buildNum.toString(), }) ); From 4ad5f1c905c246a2b6909ffa803b08c2e3d483a4 Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 27 Apr 2020 16:18:52 -0700 Subject: [PATCH 12/19] update bundles_route tests to inject buildNum everywhere --- .../bundles_route/__tests__/bundles_route.js | 69 +++++++------------ 1 file changed, 24 insertions(+), 45 deletions(-) diff --git a/src/optimize/bundles_route/__tests__/bundles_route.js b/src/optimize/bundles_route/__tests__/bundles_route.js index a99951b391aa3..902fa59b20569 100644 --- a/src/optimize/bundles_route/__tests__/bundles_route.js +++ b/src/optimize/bundles_route/__tests__/bundles_route.js @@ -33,7 +33,6 @@ import { PUBLIC_PATH_PLACEHOLDER } from '../../public_path_placeholder'; const chance = new Chance(); const outputFixture = resolve(__dirname, './fixtures/output'); const pluginNoPlaceholderFixture = resolve(__dirname, './fixtures/plugin/no_placeholder'); -const pluginPlaceholderFixture = resolve(__dirname, './fixtures/plugin/placeholder'); const randomWordsCache = new Set(); const uniqueRandomWord = () => { @@ -166,7 +165,7 @@ describe('optimizer/bundle route', () => { it('responds with exact file data', async () => { const server = createServer(); const response = await server.inject({ - url: '/bundles/image.png', + url: '/1234/bundles/image.png', }); expect(response.statusCode).to.be(200); @@ -181,7 +180,7 @@ describe('optimizer/bundle route', () => { it('responds with no content-length and exact file data', async () => { const server = createServer(); const response = await server.inject({ - url: '/bundles/no_placeholder.js', + url: '/1234/bundles/no_placeholder.js', }); expect(response.statusCode).to.be(200); @@ -195,12 +194,12 @@ describe('optimizer/bundle route', () => { }); describe('js file with placeholder', () => { - it('responds with no content-length and modified file data', async () => { + it('responds with no content-length and modifiedfile data ', async () => { const basePublicPath = `/${uniqueRandomWord()}`; const server = createServer({ basePublicPath }); const response = await server.inject({ - url: '/bundles/with_placeholder.js', + url: '/1234/bundles/with_placeholder.js', }); expect(response.statusCode).to.be(200); @@ -212,7 +211,7 @@ describe('optimizer/bundle route', () => { ); expect(response.result.indexOf(source)).to.be(-1); expect(response.result).to.be( - replaceAll(source, PUBLIC_PATH_PLACEHOLDER, `${basePublicPath}/bundles/`) + replaceAll(source, PUBLIC_PATH_PLACEHOLDER, `${basePublicPath}/1234/bundles/`) ); }); }); @@ -221,7 +220,7 @@ describe('optimizer/bundle route', () => { it('responds with no content-length and exact file data', async () => { const server = createServer(); const response = await server.inject({ - url: '/bundles/no_placeholder.css', + url: '/1234/bundles/no_placeholder.css', }); expect(response.statusCode).to.be(200); @@ -239,7 +238,7 @@ describe('optimizer/bundle route', () => { const server = createServer({ basePublicPath }); const response = await server.inject({ - url: '/bundles/with_placeholder.css', + url: '/1234/bundles/with_placeholder.css', }); expect(response.statusCode).to.be(200); @@ -248,7 +247,7 @@ describe('optimizer/bundle route', () => { expect(response.headers).to.have.property('content-type', 'text/css; charset=utf-8'); expect(response.result.indexOf(source)).to.be(-1); expect(response.result).to.be( - replaceAll(source, PUBLIC_PATH_PLACEHOLDER, `${basePublicPath}/bundles/`) + replaceAll(source, PUBLIC_PATH_PLACEHOLDER, `${basePublicPath}/1234/bundles/`) ); }); }); @@ -258,7 +257,7 @@ describe('optimizer/bundle route', () => { const server = createServer(); const response = await server.inject({ - url: '/bundles/../outside_output.js', + url: '/1234/bundles/../outside_output.js', }); expect(response.statusCode).to.be(404); @@ -275,7 +274,7 @@ describe('optimizer/bundle route', () => { const server = createServer(); const response = await server.inject({ - url: '/bundles/non_existent.js', + url: '/1234/bundles/non_existent.js', }); expect(response.statusCode).to.be(404); @@ -294,7 +293,7 @@ describe('optimizer/bundle route', () => { }); const response = await server.inject({ - url: '/bundles/with_placeholder.js', + url: '/1234/bundles/with_placeholder.js', }); expect(response.statusCode).to.be(404); @@ -314,7 +313,7 @@ describe('optimizer/bundle route', () => { sinon.assert.notCalled(createHash); const resp1 = await server.inject({ - url: '/bundles/no_placeholder.js', + url: '/1234/bundles/no_placeholder.js', }); sinon.assert.calledOnce(createHash); @@ -322,7 +321,7 @@ describe('optimizer/bundle route', () => { expect(resp1.statusCode).to.be(200); const resp2 = await server.inject({ - url: '/bundles/no_placeholder.js', + url: '/1234/bundles/no_placeholder.js', }); sinon.assert.notCalled(createHash); @@ -335,10 +334,10 @@ describe('optimizer/bundle route', () => { const [resp1, resp2] = await Promise.all([ createServer({ basePublicPath: basePublicPath1 }).inject({ - url: '/bundles/no_placeholder.js', + url: '/1234/bundles/no_placeholder.js', }), createServer({ basePublicPath: basePublicPath2 }).inject({ - url: '/bundles/no_placeholder.js', + url: '/1234/bundles/no_placeholder.js', }), ]); @@ -357,13 +356,13 @@ describe('optimizer/bundle route', () => { it('responds with 304 when etag and last modified are sent back', async () => { const server = createServer(); const resp = await server.inject({ - url: '/bundles/with_placeholder.js', + url: '/1234/bundles/with_placeholder.js', }); expect(resp.statusCode).to.be(200); const resp2 = await server.inject({ - url: '/bundles/with_placeholder.js', + url: '/1234/bundles/with_placeholder.js', headers: { 'if-modified-since': resp.headers['last-modified'], 'if-none-match': resp.headers.etag, @@ -388,7 +387,7 @@ describe('optimizer/bundle route', () => { }, ]; const responce = await createServer({ basePublicPath, npUiPluginPublicDirs }).inject({ - url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + url: '/1234/bundles/plugin/no_placeholder/no_placeholder.plugin.js', }); expect(responce.statusCode).to.be(200); @@ -397,10 +396,7 @@ describe('optimizer/bundle route', () => { expect(responce.headers['cache-control']).to.be('must-revalidate'); }); - it('creates the same "etag" header for the same content', async () => { - const basePublicPath1 = `/${uniqueRandomWord()}`; - const basePublicPath2 = `/${uniqueRandomWord()}`; - + it('creates the same "etag" header for the same content with the same basePath', async () => { const npUiPluginPublicDirs = [ { id: 'no_placeholder', @@ -408,11 +404,11 @@ describe('optimizer/bundle route', () => { }, ]; const [resp1, resp2] = await Promise.all([ - createServer({ basePublicPath: basePublicPath1, npUiPluginPublicDirs }).inject({ - url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + createServer({ basePublicPath: '', npUiPluginPublicDirs }).inject({ + url: '/1234/bundles/plugin/no_placeholder/no_placeholder.plugin.js', }), - createServer({ basePublicPath: basePublicPath2, npUiPluginPublicDirs }).inject({ - url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + createServer({ basePublicPath: '', npUiPluginPublicDirs }).inject({ + url: '/1234/bundles/plugin/no_placeholder/no_placeholder.plugin.js', }), ]); @@ -425,23 +421,6 @@ describe('optimizer/bundle route', () => { expect(resp2.headers.etag).to.be.a('string'); expect(resp1.headers.etag).to.eql(resp2.headers.etag); }); - - it('does not replace placeholder', async () => { - const basePublicPath = `/${uniqueRandomWord()}`; - - const npUiPluginPublicDirs = [ - { - id: 'placeholder', - path: pluginPlaceholderFixture, - }, - ]; - const response = await createServer({ basePublicPath, npUiPluginPublicDirs }).inject({ - url: '/bundles/1234/plugin/placeholder/placeholder.plugin.js', - }); - - expect(response.statusCode).to.be(200); - expect(response.result.includes('__REPLACE_WITH_PUBLIC_PATH__/FOO')).to.be(true); - }); }); describe('for distributable mode', () => { @@ -459,7 +438,7 @@ describe('optimizer/bundle route', () => { npUiPluginPublicDirs, isDist: true, }).inject({ - url: '/bundles/1234/plugin/no_placeholder/no_placeholder.plugin.js', + url: '/1234/bundles/plugin/no_placeholder/no_placeholder.plugin.js', }); expect(responce.statusCode).to.be(200); From b46fcd561512ae5449889ac4650b0c76055764ad Mon Sep 17 00:00:00 2001 From: spalger Date: Mon, 27 Apr 2020 17:04:22 -0700 Subject: [PATCH 13/19] fix karma config to point to right prefix --- tasks/config/karma.js | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tasks/config/karma.js b/tasks/config/karma.js index 5b3259eb77d14..e846b06718d96 100644 --- a/tasks/config/karma.js +++ b/tasks/config/karma.js @@ -25,6 +25,7 @@ import { DllCompiler } from '../../src/optimize/dynamic_dll_plugin'; const TOTAL_CI_SHARDS = 4; const ROOT = dirname(require.resolve('../../package.json')); +const buildNum = '9007199254740991'; module.exports = function(grunt) { function pickBrowser() { @@ -58,27 +59,27 @@ module.exports = function(grunt) { ...UiSharedDeps.jsDepFilenames.map( chunkFilename => - `http://localhost:5610/bundles/9007199254740991/kbn-ui-shared-deps/${chunkFilename}` + `http://localhost:5610/${buildNum}/bundles/kbn-ui-shared-deps/${chunkFilename}` ), - `http://localhost:5610/bundles/9007199254740991/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, + `http://localhost:5610/${buildNum}/bundles/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, - 'http://localhost:5610/built_assets/dlls/vendors_runtime.bundle.dll.js', + `http://localhost:5610/${buildNum}/built_assets/dlls/vendors_runtime.bundle.dll.js`, ...DllCompiler.getRawDllConfig().chunks.map( - chunk => `http://localhost:5610/built_assets/dlls/vendors${chunk}.bundle.dll.js` + chunk => `http://localhost:5610/${buildNum}/built_assets/dlls/vendors${chunk}.bundle.dll.js` ), shardNum === undefined - ? `http://localhost:5610/bundles/tests.bundle.js` - : `http://localhost:5610/bundles/tests.bundle.js?shards=${TOTAL_CI_SHARDS}&shard_num=${shardNum}`, + ? `http://localhost:5610/${buildNum}/bundles/tests.bundle.js` + : `http://localhost:5610/${buildNum}/bundles/tests.bundle.js?shards=${TOTAL_CI_SHARDS}&shard_num=${shardNum}`, - `http://localhost:5610/bundles/9007199254740991/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, + `http://localhost:5610/${buildNum}/bundles/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, // this causes tilemap tests to fail, probably because the eui styles haven't been // included in the karma harness a long some time, if ever // `http://localhost:5610/bundles/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, ...DllCompiler.getRawDllConfig().chunks.map( - chunk => `http://localhost:5610/built_assets/dlls/vendors${chunk}.style.dll.css` + chunk => `http://localhost:5610/${buildNum}/built_assets/dlls/vendors${chunk}.style.dll.css` ), - 'http://localhost:5610/bundles/tests.style.css', + `http://localhost:5610/${buildNum}/bundles/tests.style.css`, ]; } @@ -128,9 +129,9 @@ module.exports = function(grunt) { proxies: { '/tests/': 'http://localhost:5610/tests/', - '/bundles/': 'http://localhost:5610/bundles/', - '/built_assets/dlls/': 'http://localhost:5610/built_assets/dlls/', '/test_bundle/': 'http://localhost:5610/test_bundle/', + [`/${buildNum}/bundles/`]: `http://localhost:5610/${buildNum}/bundles/`, + [`/${buildNum}/built_assets/dlls/`]: `http://localhost:5610/${buildNum}/built_assets/dlls/`, }, client: { From b6e7a63a88d4b536efb4efb92cde1787c3f142d6 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 28 Apr 2020 01:39:26 -0700 Subject: [PATCH 14/19] use isDist naming throughout --- src/optimize/bundles_route/bundles_route.ts | 18 +++++++++--------- .../bundles_route/dynamic_asset_response.ts | 8 ++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 61b3dca6cb85f..089f0ca2cbf44 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -88,7 +88,7 @@ export function createBundlesRoute({ routePath: `/${buildHash}/bundles/kbn-ui-shared-deps/`, bundlesPath: UiSharedDeps.distDir, fileHashCache, - isImmutable: isDist, + isDist, }), ...npUiPluginPublicDirs.map(({ id, path }) => buildRouteForBundles({ @@ -96,7 +96,7 @@ export function createBundlesRoute({ routePath: `/${buildHash}/bundles/plugin/${id}/`, bundlesPath: path, fileHashCache, - isImmutable: isDist, + isDist, }) ), buildRouteForBundles({ @@ -104,28 +104,28 @@ export function createBundlesRoute({ routePath: `/${buildHash}/bundles/core/`, bundlesPath: fromRoot(join('src', 'core', 'target', 'public')), fileHashCache, - isImmutable: isDist, + isDist, }), buildRouteForBundles({ publicPath: `${basePublicPath}/${buildHash}/bundles/`, routePath: `/${buildHash}/bundles/`, bundlesPath: regularBundlesPath, fileHashCache, - isImmutable: isDist, + isDist, }), buildRouteForBundles({ publicPath: `${basePublicPath}/${buildHash}/built_assets/dlls/`, routePath: `/${buildHash}/built_assets/dlls/`, bundlesPath: dllBundlesPath, fileHashCache, - isImmutable: isDist, + isDist, }), buildRouteForBundles({ publicPath: `${basePublicPath}/`, routePath: `/${buildHash}/built_assets/css/`, bundlesPath: builtCssPath, fileHashCache, - isImmutable: isDist, + isDist, }), ]; } @@ -135,13 +135,13 @@ function buildRouteForBundles({ routePath, bundlesPath, fileHashCache, - isImmutable, + isDist, }: { publicPath: string; routePath: string; bundlesPath: string; fileHashCache: LruCache; - isImmutable: boolean; + isDist: boolean; }) { return { method: 'GET', @@ -163,7 +163,7 @@ function buildRouteForBundles({ bundlesPath, fileHashCache, publicPath, - isImmutable, + isDist, }); }, }, diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index 70cfb0f531836..3828bc576e865 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -62,14 +62,14 @@ export async function createDynamicAssetResponse({ bundlesPath, publicPath, fileHashCache, - isImmutable, + isDist, }: { request: Hapi.Request; h: Hapi.ResponseToolkit; bundlesPath: string; publicPath: string; fileHashCache: LruCache; - isImmutable: boolean; + isDist: boolean; }) { let fd: number | undefined; try { @@ -92,7 +92,7 @@ export async function createDynamicAssetResponse({ }); const stat = await fcb(cb => fstat(fd!, cb)); - const hash = isImmutable ? undefined : await getFileHash(fileHashCache, path, stat, fd); + const hash = isDist ? undefined : await getFileHash(fileHashCache, path, stat, fd); fd = undefined; // read stream is now responsible for fd const response = h @@ -101,7 +101,7 @@ export async function createDynamicAssetResponse({ .code(200) .type(request.server.mime.path(path).type); - if (isImmutable) { + if (isDist) { response.header('cache-control', 'max-age=31536000'); } else { response.etag(`${hash}-${publicPath}`); From 240553f237a00fa43d60ea7049788ef4c9f940b2 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 28 Apr 2020 01:39:42 -0700 Subject: [PATCH 15/19] explain magic number with variables --- src/optimize/bundles_route/dynamic_asset_response.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index 3828bc576e865..800e75cf59cfc 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -31,6 +31,11 @@ import { getFileHash } from './file_hash'; // @ts-ignore import { replacePlaceholder } from '../public_path_placeholder'; +const SECOND = 1000; +const MINUTE = 60 * SECOND; +const HOUR = 60 * MINUTE; +const DAY = 24 * HOUR; + /** * Create a Hapi response for the requested path. This is designed * to replicate a subset of the features provided by Hapi's Inert @@ -102,7 +107,7 @@ export async function createDynamicAssetResponse({ .type(request.server.mime.path(path).type); if (isDist) { - response.header('cache-control', 'max-age=31536000'); + response.header('cache-control', `max-age=${365 * DAY}`); } else { response.etag(`${hash}-${publicPath}`); response.header('cache-control', 'must-revalidate'); From 7c35408996fcbb2d4c38c938fc46da598246b47b Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 28 Apr 2020 01:46:40 -0700 Subject: [PATCH 16/19] restore replacePublicPath option from #64226 --- src/optimize/bundles_route/bundles_route.ts | 5 +++++ .../bundles_route/dynamic_asset_response.ts | 17 ++++++++++------- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index 089f0ca2cbf44..e831b15e8f8e3 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -88,6 +88,7 @@ export function createBundlesRoute({ routePath: `/${buildHash}/bundles/kbn-ui-shared-deps/`, bundlesPath: UiSharedDeps.distDir, fileHashCache, + replacePublicPath: false, isDist, }), ...npUiPluginPublicDirs.map(({ id, path }) => @@ -96,6 +97,7 @@ export function createBundlesRoute({ routePath: `/${buildHash}/bundles/plugin/${id}/`, bundlesPath: path, fileHashCache, + replacePublicPath: false, isDist, }) ), @@ -135,12 +137,14 @@ function buildRouteForBundles({ routePath, bundlesPath, fileHashCache, + replacePublicPath = true, isDist, }: { publicPath: string; routePath: string; bundlesPath: string; fileHashCache: LruCache; + replacePublicPath?: boolean; isDist: boolean; }) { return { @@ -163,6 +167,7 @@ function buildRouteForBundles({ bundlesPath, fileHashCache, publicPath, + replacePublicPath, isDist, }); }, diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index 800e75cf59cfc..06ca906a5df21 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -31,8 +31,7 @@ import { getFileHash } from './file_hash'; // @ts-ignore import { replacePlaceholder } from '../public_path_placeholder'; -const SECOND = 1000; -const MINUTE = 60 * SECOND; +const MINUTE = 60; const HOUR = 60 * MINUTE; const DAY = 24 * HOUR; @@ -67,6 +66,7 @@ export async function createDynamicAssetResponse({ bundlesPath, publicPath, fileHashCache, + replacePublicPath, isDist, }: { request: Hapi.Request; @@ -74,6 +74,7 @@ export async function createDynamicAssetResponse({ bundlesPath: string; publicPath: string; fileHashCache: LruCache; + replacePublicPath: boolean; isDist: boolean; }) { let fd: number | undefined; @@ -90,18 +91,20 @@ export async function createDynamicAssetResponse({ // the file 2 or 3 times per request it seems logical fd = await fcb(cb => open(path, 'r', cb)); + const stat = await fcb(cb => fstat(fd!, cb)); + const hash = isDist ? undefined : await getFileHash(fileHashCache, path, stat, fd); + const read = createReadStream(null as any, { fd, start: 0, autoClose: true, }); - - const stat = await fcb(cb => fstat(fd!, cb)); - const hash = isDist ? undefined : await getFileHash(fileHashCache, path, stat, fd); fd = undefined; // read stream is now responsible for fd + const content = replacePublicPath ? replacePlaceholder(read, publicPath) : read; + const response = h - .response(replacePlaceholder(read, publicPath)) + .response(content) .takeover() .code(200) .type(request.server.mime.path(path).type); @@ -118,7 +121,7 @@ export async function createDynamicAssetResponse({ if (fd) { try { await fcb(cb => close(fd!, cb)); - } catch (err) { + } catch (_) { // ignore errors from close, we already have one to report // and it's very likely they are the same } From 0beb6ab9665fee689dfa3061131e0c7da5083049 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 28 Apr 2020 01:48:48 -0700 Subject: [PATCH 17/19] replace one more instance of replacePublicPath --- src/optimize/bundles_route/bundles_route.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/optimize/bundles_route/bundles_route.ts b/src/optimize/bundles_route/bundles_route.ts index e831b15e8f8e3..cfbf09bd96804 100644 --- a/src/optimize/bundles_route/bundles_route.ts +++ b/src/optimize/bundles_route/bundles_route.ts @@ -106,6 +106,7 @@ export function createBundlesRoute({ routePath: `/${buildHash}/bundles/core/`, bundlesPath: fromRoot(join('src', 'core', 'target', 'public')), fileHashCache, + replacePublicPath: false, isDist, }), buildRouteForBundles({ From 604ab2624d00e3189bb3ff6070ea8ec1e53134fa Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 28 Apr 2020 01:52:24 -0700 Subject: [PATCH 18/19] use promisify instead of bluebird + non-null assertions --- .../bundles_route/dynamic_asset_response.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/optimize/bundles_route/dynamic_asset_response.ts b/src/optimize/bundles_route/dynamic_asset_response.ts index 06ca906a5df21..fc56ce95fc8b2 100644 --- a/src/optimize/bundles_route/dynamic_asset_response.ts +++ b/src/optimize/bundles_route/dynamic_asset_response.ts @@ -17,14 +17,14 @@ * under the License. */ +import Util from 'util'; +import Fs from 'fs'; import { resolve } from 'path'; -import { open, fstat, createReadStream, close } from 'fs'; import Hapi from 'hapi'; import LruCache from 'lru-cache'; import Boom from 'boom'; -import { fromNode as fcb } from 'bluebird'; // @ts-ignore import { getFileHash } from './file_hash'; @@ -35,6 +35,10 @@ const MINUTE = 60; const HOUR = 60 * MINUTE; const DAY = 24 * HOUR; +const asyncOpen = Util.promisify(Fs.open); +const asyncClose = Util.promisify(Fs.close); +const asyncFstat = Util.promisify(Fs.fstat); + /** * Create a Hapi response for the requested path. This is designed * to replicate a subset of the features provided by Hapi's Inert @@ -89,12 +93,12 @@ export async function createDynamicAssetResponse({ // we use and manage a file descriptor mostly because // that's what Inert does, and since we are accessing // the file 2 or 3 times per request it seems logical - fd = await fcb(cb => open(path, 'r', cb)); + fd = await asyncOpen(path, 'r'); - const stat = await fcb(cb => fstat(fd!, cb)); + const stat = await asyncFstat(fd); const hash = isDist ? undefined : await getFileHash(fileHashCache, path, stat, fd); - const read = createReadStream(null as any, { + const read = Fs.createReadStream(null as any, { fd, start: 0, autoClose: true, @@ -120,7 +124,7 @@ export async function createDynamicAssetResponse({ } catch (error) { if (fd) { try { - await fcb(cb => close(fd!, cb)); + await asyncClose(fd); } catch (_) { // ignore errors from close, we already have one to report // and it's very likely they are the same From 94690da4eaf89028487fd21538787aae7da260c9 Mon Sep 17 00:00:00 2001 From: spalger Date: Tue, 28 Apr 2020 01:55:26 -0700 Subject: [PATCH 19/19] remove one more magic number --- tasks/config/karma.js | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/tasks/config/karma.js b/tasks/config/karma.js index e846b06718d96..f87edbf04f220 100644 --- a/tasks/config/karma.js +++ b/tasks/config/karma.js @@ -25,7 +25,7 @@ import { DllCompiler } from '../../src/optimize/dynamic_dll_plugin'; const TOTAL_CI_SHARDS = 4; const ROOT = dirname(require.resolve('../../package.json')); -const buildNum = '9007199254740991'; +const buildHash = String(Number.MAX_SAFE_INTEGER); module.exports = function(grunt) { function pickBrowser() { @@ -59,27 +59,29 @@ module.exports = function(grunt) { ...UiSharedDeps.jsDepFilenames.map( chunkFilename => - `http://localhost:5610/${buildNum}/bundles/kbn-ui-shared-deps/${chunkFilename}` + `http://localhost:5610/${buildHash}/bundles/kbn-ui-shared-deps/${chunkFilename}` ), - `http://localhost:5610/${buildNum}/bundles/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, + `http://localhost:5610/${buildHash}/bundles/kbn-ui-shared-deps/${UiSharedDeps.jsFilename}`, - `http://localhost:5610/${buildNum}/built_assets/dlls/vendors_runtime.bundle.dll.js`, + `http://localhost:5610/${buildHash}/built_assets/dlls/vendors_runtime.bundle.dll.js`, ...DllCompiler.getRawDllConfig().chunks.map( - chunk => `http://localhost:5610/${buildNum}/built_assets/dlls/vendors${chunk}.bundle.dll.js` + chunk => + `http://localhost:5610/${buildHash}/built_assets/dlls/vendors${chunk}.bundle.dll.js` ), shardNum === undefined - ? `http://localhost:5610/${buildNum}/bundles/tests.bundle.js` - : `http://localhost:5610/${buildNum}/bundles/tests.bundle.js?shards=${TOTAL_CI_SHARDS}&shard_num=${shardNum}`, + ? `http://localhost:5610/${buildHash}/bundles/tests.bundle.js` + : `http://localhost:5610/${buildHash}/bundles/tests.bundle.js?shards=${TOTAL_CI_SHARDS}&shard_num=${shardNum}`, - `http://localhost:5610/${buildNum}/bundles/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, + `http://localhost:5610/${buildHash}/bundles/kbn-ui-shared-deps/${UiSharedDeps.baseCssDistFilename}`, // this causes tilemap tests to fail, probably because the eui styles haven't been // included in the karma harness a long some time, if ever // `http://localhost:5610/bundles/kbn-ui-shared-deps/${UiSharedDeps.lightCssDistFilename}`, ...DllCompiler.getRawDllConfig().chunks.map( - chunk => `http://localhost:5610/${buildNum}/built_assets/dlls/vendors${chunk}.style.dll.css` + chunk => + `http://localhost:5610/${buildHash}/built_assets/dlls/vendors${chunk}.style.dll.css` ), - `http://localhost:5610/${buildNum}/bundles/tests.style.css`, + `http://localhost:5610/${buildHash}/bundles/tests.style.css`, ]; } @@ -130,8 +132,8 @@ module.exports = function(grunt) { proxies: { '/tests/': 'http://localhost:5610/tests/', '/test_bundle/': 'http://localhost:5610/test_bundle/', - [`/${buildNum}/bundles/`]: `http://localhost:5610/${buildNum}/bundles/`, - [`/${buildNum}/built_assets/dlls/`]: `http://localhost:5610/${buildNum}/built_assets/dlls/`, + [`/${buildHash}/bundles/`]: `http://localhost:5610/${buildHash}/bundles/`, + [`/${buildHash}/built_assets/dlls/`]: `http://localhost:5610/${buildHash}/built_assets/dlls/`, }, client: {