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.

Sign up for GitHub

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

Jump to bottom

clients(lr): export primary api, presets from lr bundle #14425

Merged
merged 7 commits into from
Nov 29, 2022
Merged

clients(lr): export primary api, presets from lr bundle #14425

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e9ff38c
clients(lr): build lr bundle as umd
paulirish Oct 4, 2022
6f94886
Merge branch 'main' into umd-bundle
paulirish Nov 16, 2022
69768b7
revert bundle building changes
paulirish Nov 16, 2022
9a0875e
clients(lr): expose primary API
paulirish Nov 16, 2022
d39fb77
revert bundle building changes, pt 2
paulirish Nov 16, 2022
e9e7d88
Merge branch 'main' into umd-bundle
connorjclark Nov 28, 2022
c18bed4
Merge branch 'main' into umd-bundle
connorjclark Nov 29, 2022
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
9 changes: 5 additions & 4 deletions build/build-bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ const banner = `
* @param {{minify: boolean}=} opts
* @return {Promise<void>}
*/
async function buildBundle(entryPath, distPath, opts = {minify: true}) {
async function buildLrBundle(entryPath, distPath, opts = {minify: true}) {
Copy link
Collaborator

@connorjclark connorjclark Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This function is also used to bundle CDT via the CLI. I think you need to add to opts a format that defaults to iife.

Copy link
Member Author

Choose a reason for hiding this comment

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

yupppppppp okay i see it now.
since the use wasn't picked up by TS i missed it.
roger that, i'll make it extensible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's fair, took me a second to grok why this wasn't ok b/c of that too

Copy link
Member Author

@paulirish paulirish Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

since this CL is getting a lil bit more involved... i could also switch devtools to adopt the UMD pattern as well.. wdyt?

it'd mean we get to drop all of these in the devtools-entry:

  // @ts-expect-error
  self.X = X;

Copy link
Collaborator

Choose a reason for hiding this comment

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

sgtm. You'll probably need to disable the devtool CI check first? Might be weird given you can't roll latest to CDT just yet. Maybe you'd just need to cherry pick into branch-9 and roll from there to get CI to pass.

if (fs.existsSync(LH_ROOT + '/lighthouse-logger/node_modules')) {
throw new Error('delete `lighthouse-logger/node_modules` because it messes up rollup bundle');
}
Expand Down Expand Up @@ -231,7 +231,8 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) {
await bundle.write({
file: distPath,
banner,
format: 'iife',
format: 'umd',
name: 'lightriderBundle',
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove name / make an optional param if you need it for LR bundle

sourcemap: DEBUG,
// Suppress code splitting.
inlineDynamicImports: true,
Expand All @@ -246,7 +247,7 @@ async function cli(argv) {
// Take paths relative to cwd and build.
const [entryPath, distPath] = argv.slice(2)
.map(filePath => path.resolve(process.cwd(), filePath));
await buildBundle(entryPath, distPath, {minify: !process.env.DEBUG});
await buildLrBundle(entryPath, distPath, {minify: !process.env.DEBUG});
}

// Test if called from the CLI or as a module.
Expand All @@ -255,5 +256,5 @@ if (esMain(import.meta)) {
}

export {
buildBundle,
buildLrBundle,
paulirish marked this conversation as resolved.
Show resolved Hide resolved
};
4 changes: 2 additions & 2 deletions build/build-lightrider-bundles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import path from 'path';
import {rollup} from 'rollup';

import * as rollupPlugins from './rollup-plugins.js';
import {buildBundle} from './build-bundle.js';
import {buildLrBundle} from './build-bundle.js';
import {LH_ROOT} from '../root.js';

const distDir = path.join(LH_ROOT, 'dist', 'lightrider');
Expand All @@ -24,7 +24,7 @@ fs.mkdirSync(distDir, {recursive: true});
function buildEntryPoint() {
const inFile = `${sourceDir}/${entrySourceName}`;
const outFile = `${distDir}/${entryDistName}`;
return buildBundle(inFile, outFile, {minify: false});
return buildLrBundle(inFile, outFile, {minify: false});
}

async function buildReportGenerator() {
Expand Down
3 changes: 3 additions & 0 deletions clients/lightrider/lightrider-entry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,7 @@ if (typeof window !== 'undefined') {

export {
runLighthouseInLR,
lighthouse,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can the above manual window assignment be removed? does umd bundle assign to global for us in browser context?

Copy link
Member Author

Choose a reason for hiding this comment

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

the window assignment can be removed if we switch our LR client from window.runLighthouseInLR to window.lightriderBundle.runLighthouseInLR.
(aka: doable)

Copy link
Collaborator

@connorjclark connorjclark Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

name is necessary for umd, and also is used to namespace on the global. makes sense.

That seems like a decent change to make IMO. Although, I would just name it lightrider (or lighthouse?)

listenForStatus,
LR_PRESETS,
};