misc(build): bundle pub ads plugin for devtools #9924

brendankenny · 2019-11-05T22:52:52Z

This is basically all that's required to get a hard-coded plugin into one of our bundles. Not quite working yet...browserify is having an issue with walking and parsing transitive dependencies that I haven't resolved yet.

(+ #9936 )

brendankenny · 2019-11-05T22:53:33Z

build/build-bundle.js

+// HACK: manually include the lighthouse-plugin-publisher-ads audits.
+/** @type {Array<string>} */
+// @ts-ignore
+const pubAdsAudits = require('lighthouse-plugin-publisher-ads/plugin.js').audits.map(a => a.path);


or whatever is a good way to get this list...readdir doesn't work cause there's more than just the audits in that directory

brendankenny · 2019-11-05T22:54:30Z

build/build-bundle.js

@@ -83,6 +88,7 @@ async function browserifyFile(entryPath, distPath) {
  }

  // Expose the audits, gatherers, and computed artifacts so they can be dynamically loaded.
+  // Exposed path relative to lighthouse-core/config/config-helpers.js (where loading occurs).


important information, passed down over generations of build revisions, but only implicitly specified :)

Suggested change

// Exposed path relative to lighthouse-core/config/config-helpers.js (where loading occurs).

// Exposed path MUST be a relative path from lighthouse-core/config/config-helpers.js (where loading occurs).

doesn't have to be spec style like this but I didn't totally understand this until rereading all the code to see what it did, any complete sentence might've helped :)

brendankenny · 2019-11-05T22:55:03Z

lighthouse-core/config/config-helpers.js

-        requirePath = resolveModule(audit.path, configDir, 'audit');
+        const absolutePath = resolveModule(audit.path, configDir, 'audit');
+        // Use a relative path so bundler can easily expose it.
+        requirePath = path.relative(__dirname, absolutePath);


needs to be relative, otherwise we'd need to be browserify exposing an absolute path

I don't totally understand the comment and this line. Isn't the bundler exposing the pub audits by manually requiring in its config file?

It seems like this path of requireAudits is only hit when we're requiring in the audits as we execute the bundle where we're in brfs land and don't have real dirnames anymore. Is that right?

It seems like this path of requireAudits is only hit when we're requiring in the audits as we execute the bundle where we're in brfs land and don't have real dirnames anymore. Is that right?

right. Internal to the bundle they have some module name that can be required. Since they're required dynamically, the name they're exposed as has to be figured out at runtime, and the easiest way to do that is to use the same name that is passed to require() in non-browserified execution. The existing code was doing that with an absolute path here, and even though we could expose the modules with the same absolute path in build-bundle, those are going to be different based on what machine it's run on, etc. Hence using a relative path here, which also works non-browserified.

It's basically the dynamic require.resolve() equivalent of the ../audits/${audit.path} for the core audits a few lines above this.

brendankenny · 2019-11-05T22:55:55Z

package.json

@@ -119,6 +119,8 @@
    "isomorphic-fetch": "^2.2.1",
    "jest": "^24.3.0",
    "jsdom": "^12.2.0",
+    "lighthouse-plugin-publisher-ads": "./node_modules/lighthouse-plugin-publisher-ads-wrapper/lighthouse-plugin-publisher-ads/",
+    "lighthouse-plugin-publisher-ads-wrapper": "https://github.com/googleads/publisher-ads-lighthouse-plugin",


it would be really helpful if lighthouse-plugin-publisher-ads was published on npm so we didn't have to add the wrapper from github and then add the plugin with a relative path into the wrapper module :)

yarn errors for me:

error Package "lighthouse-plugin-publisher-ads" refers to a non-existing file '"/usr/local/google/home/cjamcl/code/lighthouse/node_modules/lighthouse-plugin-publisher-ads-wrapper/lighthouse-plugin-publisher-ads"'.

I assume this only worked for you because you initially had lighthouse-plugin-publisher-ads-wrapper installed before adding the other one.

yarn errors for me

Yeah, we'll get it published before landing. Too bad the hack isn't reproducible from package.json though.

connorjclark · 2019-11-05T22:58:51Z

clients/devtools-entry.js

@@ -14,13 +14,15 @@ const {registerLocaleData, lookupLocale} = require('../lighthouse-core/lib/i18n/

 /**
 * Return a version of the default config, filtered to only run the specified
- * categories.
+ * categories. Exclude `lighthouse-plugin-publisher-ads` to not include the


so pub ads runs even if categoryIDs doesn't include the plugin category?

I'll reword that to be less confusing :) it's trying to say it's the same as the others... It'll run if it's in categoryIDs, otherwise no

+1 to a reword :)

we should also add a test for the onlyCategories/plugins interaction, it seems non-obvious to me what the behavior should be I never really thought about it.

connorjclark · 2019-11-05T23:00:30Z

Let me know when this is quite working and I can verify it works from the Audits panel (assuming you're not setup to verify that?)

brendankenny · 2019-11-05T23:05:52Z

Let me know when this is quite working and I can verify it works from the Audits panel (assuming you're not setup to verify that?)

SG. Feel free to poke at the browserify error, otherwise I probably won't be getting to this until tomorrow.

connorjclark

Error: unsupported type for static module: LogicalExpression
at expression:

  opts.fs || fs
 while parsing file: /usr/local/google/home/cjamcl/code/lighthouse/node_modules/mkdirp/index.js while parsing file: /usr/local/google/home/cjamcl/code/lighthouse/node_modules/mkdirp/index.js

This means that there is some code that defines a module using a logical expression. (not determinable until runtime). See mkdirp.

A dirty workaround is to ignore this module.

.ignore('mkdirp')

Doing so reveals a couple transitive modules that are pulled in by ws. Obviously, that shouldn't be browserified so let's ignore that too.

.ignore('ws')

Rather than ignore these modules explicitly, we should understand why they get used now. It's clearly due to one of the pub ad audit modules. I narrowed it down to:

const {Audit} = require('lighthouse');

This breaks everything.

const Audit = require('lighthouse/lighthouse-core/audits/audit.js');

That seems to work.

why: The wrapper module has lighthouse as a direct dependency. our bundle script ignore the cri.js but via a very specific path that only matches our actual (non node_modules) cri.js. The fix above works because lighthouse/lighthouse-core/audits/audit.js doesn't include cri.js, but lighthouse does.

@paulirish link works because it allows our cri.js ignoring to work (doesn't use vendored lighthouse).

connorjclark · 2019-11-05T23:18:15Z

package.json

@@ -119,6 +119,8 @@
    "isomorphic-fetch": "^2.2.1",
    "jest": "^24.3.0",
    "jsdom": "^12.2.0",
+    "lighthouse-plugin-publisher-ads": "./node_modules/lighthouse-plugin-publisher-ads-wrapper/lighthouse-plugin-publisher-ads/",
+    "lighthouse-plugin-publisher-ads-wrapper": "https://github.com/googleads/publisher-ads-lighthouse-plugin",


yarn errors for me:

error Package "lighthouse-plugin-publisher-ads" refers to a non-existing file '"/usr/local/google/home/cjamcl/code/lighthouse/node_modules/lighthouse-plugin-publisher-ads-wrapper/lighthouse-plugin-publisher-ads"'.

I assume this only worked for you because you initially had lighthouse-plugin-publisher-ads-wrapper installed before adding the other one.

brendankenny · 2019-11-05T23:49:10Z

Rather than ignore these modules explicitly, we should understand why they get used now

Yes, that was the implicit request :P

I narrowed it down to: const {Audit} = require('lighthouse');

Ah, nice find! Yeah, that would do it. I guess we'll need the plugin to reach in more directly.

patrickhulce · 2019-11-05T23:41:10Z

build/build-bundle.js

@@ -83,6 +88,7 @@ async function browserifyFile(entryPath, distPath) {
  }

  // Expose the audits, gatherers, and computed artifacts so they can be dynamically loaded.
+  // Exposed path relative to lighthouse-core/config/config-helpers.js (where loading occurs).


Suggested change

// Exposed path relative to lighthouse-core/config/config-helpers.js (where loading occurs).

// Exposed path MUST be a relative path from lighthouse-core/config/config-helpers.js (where loading occurs).

doesn't have to be spec style like this but I didn't totally understand this until rereading all the code to see what it did, any complete sentence might've helped :)

patrickhulce · 2019-11-05T23:42:04Z

build/build-bundle.js

@@ -92,6 +98,13 @@ async function browserifyFile(entryPath, distPath) {
    bundle = bundle.require(gatherer, {expose: gatherer.replace(driverPath, '../gather/')});
  });

+  // HACK: manually include the lighthouse-plugin-publisher-ads audits.
+  if (isDevtools(entryPath)) {


kinda feels like we should resolve those functions to booleans called isDevtools and call the functions isDevtoolsEntry or something

kinda feels like we should resolve those functions to booleans called isDevtools and call the functions isDevtoolsEntry or something

yeah, I had a similar thought. Not sure why they're like that...I assume it made sense as a one-off and we've just c/p it multiple times now

patrickhulce · 2019-11-05T23:44:14Z

build/build-bundle.js

+  // HACK: manually include the lighthouse-plugin-publisher-ads audits.
+  if (isDevtools(entryPath)) {
+    pubAdsAudits.forEach(pubAdAudit => {
+      bundle = bundle.require(pubAdAudit, {expose: '../../node_modules/' + pubAdAudit});


add a note that all the audits are already prefixed with the module/plugin name?

patrickhulce · 2019-11-05T23:50:42Z

lighthouse-core/config/config-helpers.js

-        requirePath = resolveModule(audit.path, configDir, 'audit');
+        const absolutePath = resolveModule(audit.path, configDir, 'audit');
+        // Use a relative path so bundler can easily expose it.
+        requirePath = path.relative(__dirname, absolutePath);


I don't totally understand the comment and this line. Isn't the bundler exposing the pub audits by manually requiring in its config file?

It seems like this path of requireAudits is only hit when we're requiring in the audits as we execute the bundle where we're in brfs land and don't have real dirnames anymore. Is that right?

paulirish · 2019-11-05T23:54:10Z

const {Audit} = require('lighthouse');
This breaks everything.

# i tried this in the repo root
yarn link 
yarn link lighthouse

after that yarn build-devtools seemed to work!

connorjclark · 2019-11-05T23:54:48Z

My review was edited with the root cause of the bundling issue and a workaround.

brendankenny · 2019-11-06T05:19:54Z

@paulirish link works because it allows our cri.js ignoring to work (doesn't use vendored lighthouse).

I haven't done the bundle analysis yet, but that sounds like what we might want then (we don't want duplicate lighthouse files in our bundle).

But there's a way more evil way of doing this :)

"resolutions": {
   "**/lighthouse": "file:."
}

🔥 😈 😈 😈 🔥

paulirish · 2019-11-06T05:41:54Z

seems good.

brendankenny · 2019-11-06T23:43:18Z

This appears to be working and the bundle looks good. It does currently require a

yarn link 
yarn link lighthouse

in the base directory to get a symlink (resolutions copies which we don't want), but we can figure out the workflow.

@connorjclark should be good to try out and we can work on build ergonomics.

brendankenny · 2019-11-06T23:48:41Z

@connorjclark should be good to try out and we can work on build ergonomics.

It might actually need a few more dynamically included files... Maybe plugin.js for the plugin config? I can add that

lighthouse-core/config/config-helpers.js

googlebot added the cla: yes label Nov 5, 2019

brendankenny commented Nov 5, 2019

View reviewed changes

connorjclark reviewed Nov 5, 2019

View reviewed changes

connorjclark requested changes Nov 5, 2019

View reviewed changes

patrickhulce reviewed Nov 5, 2019

View reviewed changes

brendankenny added 2 commits November 6, 2019 18:02

WIP: include plugin in dt build

948ee09

feedback

082715b

brendankenny force-pushed the dtplugin branch from b7fee07 to 082715b Compare November 6, 2019 23:42

vercel bot deployed to staging November 6, 2019 23:42 View deployment

connorjclark added the 6.0 label Nov 8, 2019

bundling pubads for devtools (#9936)

55839fd

vercel bot deployed to staging November 9, 2019 03:49 View deployment

connorjclark changed the title ~~WIP: include plugin in dt build~~ misc(build): bundle pub ads plugin for devtools Nov 9, 2019

paulirish approved these changes Nov 9, 2019

View reviewed changes

lighthouse-core/config/config-helpers.js Outdated Show resolved Hide resolved

paulirish marked this pull request as ready for review November 9, 2019 03:55

prepare to land

b6f347a

vercel bot temporarily deployed to staging November 9, 2019 03:59 Inactive

comment

85dbdfd

vercel bot deployed to staging November 9, 2019 04:00 View deployment

link

e2fa729

vercel bot deployed to staging November 9, 2019 05:39 View deployment

ryan-ally mentioned this pull request Jul 4, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ryan-ally/lighthouse#37

Open

kalpatarurudra mentioned this pull request Jul 5, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 kalpatarurudra/lighthouse#23

Open

MaxMood96 mentioned this pull request Jul 5, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 MaxMood96/lighthouse#99

Open

MaxMood96 mentioned this pull request Nov 26, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 MaxMood96/lighthouse#103

Open

ryan-ally mentioned this pull request Nov 27, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ryan-ally/lighthouse#43

Open

cr-icon mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 cr-icon/lighthouse#38

Open

HijadelUFO mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 HijadelUFO/lighthouse#65

Open

ashubagri mentioned this pull request Dec 1, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ashubagri/lighthouse#13

Open

kalpatarurudra mentioned this pull request Dec 1, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 kalpatarurudra/lighthouse#29

Open

MaxMood96 mentioned this pull request Dec 2, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 MaxMood96/lighthouse#112

Open

thexdesk mentioned this pull request Dec 3, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 thexdesk/lighthouse#34

Open

HijadelUFO mentioned this pull request Dec 3, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 HijadelUFO/lighthouse#70

Open

ryan-ally mentioned this pull request Dec 3, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ryan-ally/lighthouse#52

Open

cr-icon mentioned this pull request Dec 5, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 cr-icon/lighthouse#42

Open

cr-icon mentioned this pull request Dec 13, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 cr-icon/lighthouse#43

Open

ashubagri mentioned this pull request Dec 15, 2023

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ashubagri/lighthouse#19

Open

kalpatarurudra mentioned this pull request Jan 2, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 kalpatarurudra/lighthouse#31

Open

MaxMood96 mentioned this pull request Jan 5, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 MaxMood96/lighthouse#116

Open

thexdesk mentioned this pull request Feb 25, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 thexdesk/lighthouse#36

Open

HijadelUFO mentioned this pull request Mar 1, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 HijadelUFO/lighthouse#73

Open

ryan-ally mentioned this pull request Mar 6, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ryan-ally/lighthouse#57

Open

kalpatarurudra mentioned this pull request May 14, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 kalpatarurudra/lighthouse#34

Open

cr-icon mentioned this pull request May 14, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 cr-icon/lighthouse#46

Open

HijadelUFO mentioned this pull request May 15, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 HijadelUFO/lighthouse#76

Open

ryan-ally mentioned this pull request May 15, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ryan-ally/lighthouse#64

Open

kalpatarurudra mentioned this pull request May 15, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 kalpatarurudra/lighthouse#35

Open

cr-icon mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 cr-icon/lighthouse#47

Open

ashubagri mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 ashubagri/lighthouse#24

Open

MaxMood96 mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 MaxMood96/lighthouse#124

Open

thexdesk mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 24.9.0 to 27.0.0 thexdesk/lighthouse#38

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc(build): bundle pub ads plugin for devtools #9924

misc(build): bundle pub ads plugin for devtools #9924

brendankenny commented Nov 5, 2019 •

edited by connorjclark

Loading

brendankenny Nov 5, 2019

brendankenny Nov 5, 2019

patrickhulce Nov 5, 2019

brendankenny Nov 5, 2019

patrickhulce Nov 5, 2019

brendankenny Nov 6, 2019

brendankenny Nov 5, 2019

connorjclark Nov 5, 2019

patrickhulce Nov 5, 2019

brendankenny Nov 5, 2019

connorjclark Nov 5, 2019

brendankenny Nov 5, 2019

patrickhulce Nov 5, 2019

connorjclark commented Nov 5, 2019

brendankenny commented Nov 5, 2019

connorjclark left a comment •

edited

Loading

connorjclark Nov 5, 2019

brendankenny commented Nov 5, 2019

patrickhulce Nov 5, 2019

patrickhulce Nov 5, 2019

brendankenny Nov 6, 2019

patrickhulce Nov 5, 2019

patrickhulce Nov 5, 2019

paulirish commented Nov 5, 2019

connorjclark commented Nov 5, 2019 •

edited

Loading

brendankenny commented Nov 6, 2019

paulirish commented Nov 6, 2019

brendankenny commented Nov 6, 2019

brendankenny commented Nov 6, 2019

	// Exposed path relative to lighthouse-core/config/config-helpers.js (where loading occurs).
	// Exposed path MUST be a relative path from lighthouse-core/config/config-helpers.js (where loading occurs).

misc(build): bundle pub ads plugin for devtools #9924

misc(build): bundle pub ads plugin for devtools #9924

Conversation

brendankenny commented Nov 5, 2019 • edited by connorjclark Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Nov 5, 2019

brendankenny commented Nov 5, 2019

connorjclark left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Nov 5, 2019

connorjclark commented Nov 5, 2019 • edited Loading

brendankenny commented Nov 6, 2019

paulirish commented Nov 6, 2019

brendankenny commented Nov 6, 2019

brendankenny commented Nov 6, 2019

brendankenny commented Nov 5, 2019 •

edited by connorjclark

Loading

connorjclark left a comment •

edited

Loading

connorjclark commented Nov 5, 2019 •

edited

Loading