Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misc(build): bundle with esbuild minification instead of terser #15283

Merged
merged 3 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions build/build-bundle.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import esbuild from 'esbuild';
import PubAdsPlugin from 'lighthouse-plugin-publisher-ads';
// @ts-expect-error: plugin has no types.
import SoftNavPlugin from 'lighthouse-plugin-soft-navigation';
import * as terser from 'terser';

import * as plugins from './esbuild-plugins.js';
import {Runner} from '../core/runner.js';
Expand Down Expand Up @@ -146,13 +145,11 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) {

const result = await esbuild.build({
entryPoints: [entryPath],
outfile: distPath,
write: false,
format: 'iife',
charset: 'utf8',
bundle: true,
// For now, we defer to terser.
minify: false,
minify: opts.minify,
treeShaking: true,
sourcemap: DEBUG,
banner: {js: banner},
Expand Down Expand Up @@ -250,26 +247,14 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) {
],
});

let code = result.outputFiles[0].text;
const code = result.outputFiles[0].text;

// Just make sure the above shimming worked.
if (code.includes('inflate_fast')) {
throw new Error('Expected zlib inflate code to have been removed');
}

// Ideally we'd let esbuild minify, but we need to disable variable name mangling otherwise
// code generated dynamically to run inside the browser (pageFunctions) breaks. For example,
// the `truncate` function is unable to properly reference `Util`.
if (opts.minify) {
code = (await terser.minify(result.outputFiles[0].text, {
mangle: false,
format: {
max_line_len: 1000,
},
})).code || '';
}

await fs.promises.writeFile(result.outputFiles[0].path, code);
await fs.promises.writeFile(distPath, code);
}

/**
Expand Down
17 changes: 12 additions & 5 deletions core/gather/driver/execution-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class ExecutionContext {
${ExecutionContext._cachedNativesPreamble};
globalThis.__lighthouseExecutionContextUniqueIdentifier =
${uniqueExecutionContextIdentifier};
${pageFunctions.esbuildFunctionNameStubString}
${pageFunctions.esbuildFunctionWrapperString}
return new Promise(function (resolve) {
return Promise.resolve()
.then(_ => ${expression})
Expand Down Expand Up @@ -277,13 +277,20 @@ class ExecutionContext {
* @return {string}
*/
static serializeDeps(deps) {
deps = [pageFunctions.esbuildFunctionNameStubString, ...deps || []];
deps = [pageFunctions.esbuildFunctionWrapperString, ...deps || []];
return deps.map(dep => {
if (typeof dep === 'function') {
// esbuild will change the actual function name (ie. function actualName() {})
// always, despite minification settings, but preserve the real name in `actualName.name`
// (see esbuildFunctionNameStubString).
return `const ${dep.name} = ${dep}`;
// always, and preserve the real name in `actualName.name`.
// See esbuildFunctionWrapperString.
const output = dep.toString();
const runtimeName = pageFunctions.getRuntimeFunctionName(dep);
if (runtimeName !== dep.name) {
// In addition to exposing the mangled name, also expose the original as an alias.
return `${output}; const ${dep.name} = ${runtimeName};`;
} else {
return output;
}
} else {
return dep;
}
Expand Down
10 changes: 7 additions & 3 deletions core/gather/gatherers/trace-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {ProcessedNavigation} from '../../computed/processed-navigation.js';
import {LighthouseError} from '../../lib/lh-error.js';
import {Responsiveness} from '../../computed/metrics/responsiveness.js';
import {CumulativeLayoutShift} from '../../computed/metrics/cumulative-layout-shift.js';
import {ExecutionContext} from '../driver/execution-context.js';

/** @typedef {{nodeId: number, score?: number, animations?: {name?: string, failureReasonsMask?: number, unsupportedProperties?: string[]}[], type?: string}} TraceElementData */

Expand Down Expand Up @@ -284,12 +285,15 @@ class TraceElements extends BaseGatherer {
try {
const objectId = await resolveNodeIdToObjectId(session, backendNodeId);
if (!objectId) continue;

const deps = ExecutionContext.serializeDeps([
pageFunctions.getNodeDetails,
getNodeDetailsData,
]);
response = await session.sendCommand('Runtime.callFunctionOn', {
objectId,
functionDeclaration: `function () {
${pageFunctions.esbuildFunctionNameStubString}
${getNodeDetailsData.toString()};
${pageFunctions.getNodeDetails};
${deps}
return getNodeDetailsData.call(this);
}`,
returnByValue: true,
Expand Down
88 changes: 78 additions & 10 deletions core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* 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 {createRequire} from 'module';

import {Util} from '../../shared/util.js';

/**
Expand Down Expand Up @@ -50,7 +52,7 @@ function wrapRuntimeEvalErrorInBrowser(err) {

/**
* @template {string} T
* @param {T} selector Optional simple CSS selector to filter nodes on.
* @param {T=} selector Optional simple CSS selector to filter nodes on.
* Combinators are not supported.
* @return {Array<ParseSelector<T>>}
*/
Expand Down Expand Up @@ -513,39 +515,104 @@ function truncate(string, characterLimit) {
return Util.truncate(string, characterLimit);
}

function isBundledEnvironment() {
// If we're in DevTools or LightRider, we are definitely bundled.
// TODO: refactor and delete `global.isDevtools`.
if (global.isDevtools || global.isLightrider) return true;

const require = createRequire(import.meta.url);

try {
// Not foolproof, but `lighthouse-logger` is a dependency of lighthouse that should always be resolvable.
// `require.resolve` will only throw in atypical/bundled environments.
require.resolve('lighthouse-logger');
return false;
} catch (err) {
return true;
}
}

// This is to support bundled lighthouse.
// esbuild calls every function with a builtin `__name` (since we set keepNames: true),
// whose purpose is to store the real name of the function so that esbuild can rename it to avoid
// collisions. Anywhere we inject dynamically generated code at runtime for the browser to process,
// we must manually include this function (because esbuild only does so once at the top scope of
// the bundle, which is irrelevant for code executed in the browser).
const esbuildFunctionNameStubString = 'var __name=(fn)=>fn;';
// When minified, esbuild will mangle the name of this wrapper function, so we need to determine what it
// is at runtime in order to recreate it within the page.
const esbuildFunctionWrapperString = createEsbuildFunctionWrapper();

/** @type {string} */
const truncateRawString = truncate.toString();
truncate.toString = () => `function truncate(string, characterLimit) {
function createEsbuildFunctionWrapper() {
if (!isBundledEnvironment()) {
return '';
}

const functionAsString = (()=>{
// eslint-disable-next-line no-unused-vars
const a = ()=>{};
}).toString()
// When not minified, esbuild annotates the call to this function wrapper with PURE.
// We know further that the name of the wrapper function should be `__name`, but let's not
// hardcode that. Remove the PURE annotation to simplify the regex.
.replace('/* @__PURE__ */', '');
const functionStringMatch = functionAsString.match(/=\s*([\w_]+)\(/);
if (!functionStringMatch) {
throw new Error('Could not determine esbuild function wrapper name');
}

/**
* @param {Function} fn
* @param {string} value
*/
const esbuildFunctionWrapper = (fn, value) =>
Object.defineProperty(fn, 'name', {value, configurable: true});
const wrapperFnName = functionStringMatch[1];
return `let ${wrapperFnName}=${esbuildFunctionWrapper}`;
}
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

/**
* @param {Function} fn
* @return {string}
*/
function getRuntimeFunctionName(fn) {
const match = fn.toString().match(/function ([\w$]+)/);
if (!match) throw new Error(`could not find function name for: ${fn}`);
return match[1];
}

// We setup a number of our page functions to automatically include their dependencies.
// Because of esbuild bundling, we must refer to the actual (mangled) runtime function name.
/** @type {Record<string, string>} */
const names = {
truncate: getRuntimeFunctionName(truncate),
getNodeLabel: getRuntimeFunctionName(getNodeLabel),
getOuterHTMLSnippet: getRuntimeFunctionName(getOuterHTMLSnippet),
getNodeDetails: getRuntimeFunctionName(getNodeDetails),
};
Copy link
Member

Choose a reason for hiding this comment

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

nit: my preference would be for these to be standalone variables (e.g. truncateName). Is there a a a reason these are attached to a record object. It's kind confusing since names only contains the name of some functions.

Copy link
Collaborator Author

@connorjclark connorjclark Jul 19, 2023

Choose a reason for hiding this comment

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

Is there a a a reason these are attached to a record object.

The reason is just organization.


truncate.toString = () => `function ${names.truncate}(string, characterLimit) {
const Util = { ${Util.truncate} };
return (${truncateRawString})(string, characterLimit);
return Util.truncate(string, characterLimit);
}`;

/** @type {string} */
const getNodeLabelRawString = getNodeLabel.toString();
getNodeLabel.toString = () => `function getNodeLabel(element) {
getNodeLabel.toString = () => `function ${names.getNodeLabel}(element) {
${truncate};
return (${getNodeLabelRawString})(element);
}`;

/** @type {string} */
const getOuterHTMLSnippetRawString = getOuterHTMLSnippet.toString();
// eslint-disable-next-line max-len
getOuterHTMLSnippet.toString = () => `function getOuterHTMLSnippet(element, ignoreAttrs = [], snippetCharacterLimit = 500) {
getOuterHTMLSnippet.toString = () => `function ${names.getOuterHTMLSnippet}(element, ignoreAttrs = [], snippetCharacterLimit = 500) {
${truncate};
return (${getOuterHTMLSnippetRawString})(element, ignoreAttrs, snippetCharacterLimit);
}`;

/** @type {string} */
const getNodeDetailsRawString = getNodeDetails.toString();
getNodeDetails.toString = () => `function getNodeDetails(element) {
getNodeDetails.toString = () => `function ${names.getNodeDetails}(element) {
${truncate};
${getNodePath};
${getNodeSelector};
Expand All @@ -568,5 +635,6 @@ export const pageFunctions = {
wrapRequestIdleCallback,
getBoundingClientRect,
truncate,
esbuildFunctionNameStubString,
esbuildFunctionWrapperString,
getRuntimeFunctionName,
};
14 changes: 7 additions & 7 deletions core/test/gather/driver/execution-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,11 @@ const performance = globalThis.__nativePerformance || globalThis.performance;
const fetch = globalThis.__nativeFetch || globalThis.fetch;
globalThis.__lighthouseExecutionContextUniqueIdentifier =
undefined;
var __name=(fn)=>fn;

return new Promise(function (resolve) {
return Promise.resolve()
.then(_ => (() => {
var __name=(fn)=>fn;

return (function main(value) {
return value;
})(1);
Expand Down Expand Up @@ -274,7 +274,7 @@ const fetch = globalThis.__nativeFetch || globalThis.fetch;

const code = mockFn.mock.calls[0][0];
expect(trimTrailingWhitespace(code)).toBe(`(() => {
var __name=(fn)=>fn;

return (function mainFn(value) {
return value;
})(1);
Expand All @@ -297,7 +297,7 @@ const fetch = globalThis.__nativeFetch || globalThis.fetch;

const code = mockFn.mock.calls[0][0];
expect(trimTrailingWhitespace(code)).toBe(`(() => {
var __name=(fn)=>fn;

return ((value) => {
return value;
})(1);
Expand Down Expand Up @@ -339,11 +339,11 @@ const fetch = globalThis.__nativeFetch || globalThis.fetch;

const code = mockFn.mock.calls[0][0];
expect(trimTrailingWhitespace(code)).toEqual(`(() => {
var __name=(fn)=>fn;
const abs = function abs(val) {

function abs(val) {
return Math.abs(val);
}
const square = function square(val) {
function square(val) {
return val * val;
}
return (function mainFn({a, b}, passThru) {
Expand Down