-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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): replace rollup with esbuild #15239
Merged
Merged
Changes from all commits
Commits
Show all changes
79 commits
Select commit
Hold shift + click to select a range
1722e38
report
connorjclark 1901cf9
gh-pages
connorjclark 12301ac
wip: build lr bundles
connorjclark ffc3bad
fix lr buildReportGenerator plugin order
connorjclark 8720d6c
build-bundle pretty much done
connorjclark cf45b60
rm build-report-es.js
connorjclark aff685a
dt report resources
connorjclark 45a6583
smokehouse bundle
connorjclark 9273f70
extension
connorjclark cb0847c
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark cf101f9
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark a4a3321
fix lr gen
connorjclark 5d802e9
improve
connorjclark 10720cb
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 65e62fc
preloads
connorjclark a7f960f
charset
connorjclark 2954458
include a working node polyfill plugin
connorjclark 47ce129
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark a876857
tweak
connorjclark 0392b62
move to third-party
connorjclark b6ff768
rm rollup; fix inflate shim
connorjclark 0f4e50c
better document replaceModules plugin
connorjclark d581317
allow return null in partial loader
connorjclark 5bdd19c
big wip - page fns
connorjclark 2062ab0
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark eb121e5
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 445874f
wip
connorjclark 90e5490
page fns need a refactor...
connorjclark ff801e9
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark a15cf24
rm
connorjclark 6cdba38
just use terser for minify
connorjclark 9eddca6
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 7a4f6fd
tests: save smokehouse failures, and impove bundle runner logging
connorjclark 3d06234
Merge branch 'improve-smokehouse-failing' into build-esbuild
connorjclark 2f2f25e
fix __name
connorjclark bc1af61
updates
connorjclark 609e15d
latest esbuild
connorjclark ccffe5f
remove unused text replacements
connorjclark 4221b11
clean up build-bundle a bit
connorjclark bebd5cc
add comments to replace plugin
connorjclark 6c7ec5e
make umd plugin
connorjclark 6b0adc4
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark bcf62e3
delete commented out code
connorjclark c2531f4
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 133f3c7
remove unused code; fix smokehouse bundle
connorjclark a6277f2
fix umd plugin writing a <stdout> file
connorjclark 924b7a4
fix exec context test
connorjclark 681febf
fix types
connorjclark 0ce0745
fix umd bundle
connorjclark 60e521c
incl assert/strict for smokehouse bundle
connorjclark 3e6cc60
fix assert/strict for smokehouse bundle
connorjclark 62d8995
support nested modules for umd
connorjclark d7f76b3
fix types
connorjclark af5ca19
strictEqual
connorjclark 9b59988
i hate modules
connorjclark 4a6f0f9
lets just use assert
connorjclark 502076d
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 9c9bf58
fix merge error
connorjclark 0860d8f
dont need this
connorjclark 4c76a8e
fix eval on new doc script
connorjclark b512fae
fix exec tests
connorjclark f7a0698
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 0a27e45
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 907739a
fix process polyfilling
connorjclark bbcd02b
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark d893d23
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark ae6bf17
bring back those zlib things
connorjclark 853f9e9
use inject
connorjclark 6370622
forgot that
connorjclark 419e423
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 3c8c2e3
use esbuild to minify viewer report gen
connorjclark 7d4f200
rm old plugin
connorjclark 2eb0367
move chrome launch to before injecting bundled lh for test bundle
connorjclark a69635b
restore process
connorjclark 80db263
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 2e98743
bad merge
connorjclark 5432245
Merge remote-tracking branch 'origin/main' into build-esbuild
connorjclark 8e89da1
empty commit bc all of gha just broke
connorjclark 9e70bdc
comment
connorjclark File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,16 +15,18 @@ import {execSync} from 'child_process'; | |
import {createRequire} from 'module'; | ||
|
||
import esMain from 'es-main'; | ||
import {rollup} from 'rollup'; | ||
import esbuild from 'esbuild'; | ||
// @ts-expect-error: plugin has no types. | ||
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 rollupPlugins from './rollup-plugins.js'; | ||
import * as plugins from './esbuild-plugins.js'; | ||
import {Runner} from '../core/runner.js'; | ||
import {LH_ROOT} from '../root.js'; | ||
import {readJson} from '../core/test/test-utils.js'; | ||
import {nodeModulesPolyfillPlugin} from '../third-party/esbuild-plugins-polyfills/esbuild-polyfills.js'; | ||
|
||
const require = createRequire(import.meta.url); | ||
|
||
|
@@ -81,10 +83,6 @@ const banner = ` | |
* @return {Promise<void>} | ||
*/ | ||
async function buildBundle(entryPath, distPath, opts = {minify: true}) { | ||
if (fs.existsSync(LH_ROOT + '/lighthouse-logger/node_modules')) { | ||
throw new Error('delete `lighthouse-logger/node_modules` because it messes up rollup bundle'); | ||
} | ||
|
||
// List of paths (absolute / relative to config-helpers.js) to include | ||
// in bundle and make accessible via config-helpers.js `requireWrapper`. | ||
const dynamicModulePaths = [ | ||
|
@@ -110,12 +108,20 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) { | |
}).join(',\n'); | ||
|
||
/** @type {Record<string, string>} */ | ||
const shimsObj = {}; | ||
const shimsObj = { | ||
// zlib's decompression code is very large and we don't need it. | ||
// We export empty functions, instead of an empty module, simply to silence warnings | ||
// about no exports. | ||
'__zlib-lib/inflate': ` | ||
export function inflateInit2() {}; | ||
export function inflate() {}; | ||
export function inflateEnd() {}; | ||
export function inflateReset() {}; | ||
`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a fix we could upstream to rollup? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This must have been necessary for a previous iteration of the build script. I reduced it to an empty string just fine. |
||
}; | ||
|
||
const modulesToIgnore = [ | ||
'puppeteer-core', | ||
'intl-pluralrules', | ||
'intl', | ||
'pako/lib/zlib/inflate.js', | ||
'@sentry/node', | ||
'source-map', | ||
|
@@ -125,55 +131,37 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) { | |
// Don't include the stringified report in DevTools - see devtools-report-assets.js | ||
// Don't include in Lightrider - HTML generation isn't supported, so report assets aren't needed. | ||
if (isDevtools(entryPath) || isLightrider(entryPath)) { | ||
shimsObj[require.resolve('../report/generator/report-assets.js')] = | ||
shimsObj[`${LH_ROOT}/report/generator/report-assets.js`] = | ||
'export const reportAssets = {}'; | ||
} | ||
|
||
// Don't include locales in DevTools. | ||
if (isDevtools(entryPath)) { | ||
shimsObj['./locales.js'] = 'export const locales = {};'; | ||
shimsObj[`${LH_ROOT}/shared/localization/locales.js`] = 'export const locales = {};'; | ||
} | ||
|
||
for (const modulePath of modulesToIgnore) { | ||
shimsObj[modulePath] = 'export default {}'; | ||
} | ||
|
||
const bundle = await rollup({ | ||
input: entryPath, | ||
context: 'globalThis', | ||
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, | ||
treeShaking: true, | ||
sourcemap: DEBUG, | ||
banner: {js: banner}, | ||
// Because of page-functions! | ||
keepNames: true, | ||
inject: ['./build/process-global.js'], | ||
/** @type {esbuild.Plugin[]} */ | ||
plugins: [ | ||
rollupPlugins.replace({ | ||
delimiters: ['', ''], | ||
values: { | ||
'/* BUILD_REPLACE_BUNDLED_MODULES */': `[\n${bundledMapEntriesCode},\n]`, | ||
// This package exports to default in a way that causes Rollup to get confused, | ||
// resulting in MessageFormat being undefined. | ||
'require(\'intl-messageformat\').default': 'require(\'intl-messageformat\')', | ||
// Below we replace lighthouse-logger with a local copy, which is ES modules. Need | ||
// to change every require of the package to reflect this. | ||
'require(\'lighthouse-logger\');': 'require(\'lighthouse-logger\').default;', | ||
// Rollup doesn't replace this, so let's manually change it to false. | ||
'require.main === module': 'false', | ||
// TODO: Use globalThis directly. | ||
'global.isLightrider': 'globalThis.isLightrider', | ||
'global.isDevtools': 'globalThis.isDevtools', | ||
// For some reason, `shim` doesn't work to force this module to return false, so instead | ||
// just replace usages of it with false. | ||
'esMain(import.meta)': 'false', | ||
'import esMain from \'es-main\'': '', | ||
// By default Rollup converts `import.meta` to a big mess of `document.currentScript && ...`, | ||
// and uses the output name as the url. Instead, do a simpler conversion and use the | ||
// module path. | ||
'import.meta': (id) => `{url: '${path.relative(LH_ROOT, id)}'}`, | ||
}, | ||
}), | ||
rollupPlugins.alias({ | ||
entries: { | ||
'debug': require.resolve('debug/src/browser.js'), | ||
'lighthouse-logger': require.resolve('../lighthouse-logger/index.js'), | ||
}, | ||
}), | ||
rollupPlugins.shim({ | ||
plugins.replaceModules({ | ||
...shimsObj, | ||
'url': ` | ||
export const URL = globalThis.URL; | ||
|
@@ -189,59 +177,99 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) { | |
}; | ||
}; | ||
`, | ||
}, { | ||
// buildBundle is used in a lot of different contexts. Some share the same modules | ||
// that need to be replaced, but others don't use those modules at all. | ||
disableUnusedError: true, | ||
}), | ||
rollupPlugins.json(), | ||
rollupPlugins.removeModuleDirCalls(), | ||
rollupPlugins.inlineFs({ | ||
verbose: Boolean(process.env.DEBUG), | ||
ignorePaths: [ | ||
require.resolve('puppeteer-core/lib/esm/puppeteer/common/Page.js'), | ||
], | ||
}), | ||
rollupPlugins.commonjs({ | ||
// https://github.com/rollup/plugins/issues/922 | ||
ignoreGlobal: true, | ||
}), | ||
rollupPlugins.nodePolyfills(), | ||
rollupPlugins.nodeResolve({preferBuiltins: true}), | ||
// Rollup sees the usages of these functions in page functions (ex: see AnchorElements) | ||
// and treats them as globals. Because the names are "taken" by the global, Rollup renames | ||
// the actual functions (getNodeDetails$1). The page functions expect a certain name, so | ||
// here we undo what Rollup did. | ||
rollupPlugins.postprocess([ | ||
[/getBoundingClientRect\$1/, 'getBoundingClientRect'], | ||
[/getElementsInDocument\$1/, 'getElementsInDocument'], | ||
[/getNodeDetails\$1/, 'getNodeDetails'], | ||
[/getRectCenterPoint\$1/, 'getRectCenterPoint'], | ||
[/isPositionFixed\$1/, 'isPositionFixed'], | ||
nodeModulesPolyfillPlugin(), | ||
plugins.bulkLoader([ | ||
// TODO: when we used rollup, various things were tree-shaken out before inlineFs did its | ||
// thing. Now treeshaking only happens at the end, so the plugin sees more cases than it | ||
// did before. Some of those new cases emit warnings. Safe to ignore, but should be | ||
// resolved eventually. | ||
plugins.partialLoaders.inlineFs({ | ||
verbose: Boolean(process.env.DEBUG), | ||
ignorePaths: [require.resolve('puppeteer-core/lib/esm/puppeteer/common/Page.js')], | ||
}), | ||
plugins.partialLoaders.rmGetModuleDirectory, | ||
plugins.partialLoaders.replaceText({ | ||
'/* BUILD_REPLACE_BUNDLED_MODULES */': `[\n${bundledMapEntriesCode},\n]`, | ||
// TODO: Use globalThis directly. | ||
'global.isLightrider': 'globalThis.isLightrider', | ||
'global.isDevtools': 'globalThis.isDevtools', | ||
// By default esbuild converts `import.meta` to an empty object. | ||
// We need at least the url property for i18n things. | ||
/** @param {string} id */ | ||
'import.meta': (id) => `{url: '${path.relative(LH_ROOT, id)}'}`, | ||
}), | ||
]), | ||
opts.minify && rollupPlugins.terser({ | ||
ecma: 2019, | ||
output: { | ||
comments: (node, comment) => { | ||
const text = comment.value; | ||
if (text.includes('The Lighthouse Authors') && comment.line > 1) return false; | ||
return /@ts-nocheck - Prevent tsc|@preserve|@license|@cc_on|^!/i.test(text); | ||
}, | ||
max_line_len: 1000, | ||
{ | ||
name: 'alias', | ||
setup({onResolve}) { | ||
onResolve({filter: /\.*/}, (args) => { | ||
/** @type {Record<string, string>} */ | ||
const entries = { | ||
'debug': require.resolve('debug/src/browser.js'), | ||
'lighthouse-logger': require.resolve('../lighthouse-logger/index.js'), | ||
}; | ||
if (args.path in entries) { | ||
return {path: entries[args.path]}; | ||
} | ||
}); | ||
}, | ||
// The config relies on class names for gatherers. | ||
keep_classnames: true, | ||
// Runtime.evaluate errors if function names are elided. | ||
keep_fnames: true, | ||
}), | ||
}, | ||
{ | ||
name: 'postprocess', | ||
setup({onEnd}) { | ||
onEnd(result => { | ||
if (!result.outputFiles) throw new Error(); | ||
|
||
let code = result.outputFiles[0].text; | ||
|
||
// Get rid of our extra license comments. | ||
// https://stackoverflow.com/a/35923766 | ||
const re = /\/\*\*\s*\n([^*]|(\*(?!\/)))*\*\/\n/g; | ||
let hasSeenFirst = false; | ||
code = code.replace(re, (match) => { | ||
if (match.includes('@license') && match.match(/Lighthouse Authors|Google/)) { | ||
if (hasSeenFirst) { | ||
return ''; | ||
} | ||
|
||
hasSeenFirst = true; | ||
} | ||
|
||
return match; | ||
}); | ||
|
||
result.outputFiles[0].contents = new TextEncoder().encode(code); | ||
}); | ||
}, | ||
}, | ||
], | ||
}); | ||
|
||
await bundle.write({ | ||
file: distPath, | ||
banner, | ||
format: 'iife', | ||
sourcemap: DEBUG, | ||
// Suppress code splitting. | ||
inlineDynamicImports: true, | ||
}); | ||
await bundle.close(); | ||
let 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); | ||
} | ||
|
||
/** | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely concerned if changing our bundler introduces flakiness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option was present on the
test-bundle
script already.It's not good to apply default options to the script here. I certainly did not expect it. Note that I added this flag to CI invocation, which is where it matters.