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

Embedding in another extension #7350

Closed
benjamingr opened this issue Feb 28, 2019 · 14 comments
Closed

Embedding in another extension #7350

benjamingr opened this issue Feb 28, 2019 · 14 comments

Comments

@benjamingr
Copy link

Feature request summary

Hey, I think it could be really nice if there was a way to embed lighthouse in another extension.

We're developing an extension that already utilizes the devtools protocol and opens chrome.debugger.

Currently I was thinking about embedding an audit through the programmatic interface and CLI tool but it would be much nicer if there was a way to (from a Chrome extension context) pass a connected chrome.debugger tabId (or something similar) and embed lighthouse into another extension.

What is the motivation or use case for changing this?

Improving the UI/UX of the extension and being able to use lighthouse directly instead of reusing parts of it or giving users a subpar solution requiring a cli tool.

How is this beneficial to Lighthouse?

Bigger/better ecosystem and motivation for more extensions to get into the "care about site performance" field.

@patrickhulce
Copy link
Collaborator

I'm not sure what exactly you're feature requesting from us. You can use the browserified extension script we already make for our extension today.

@benjamingr
Copy link
Author

benjamingr commented Feb 28, 2019

@patrickhulce so, I ended up looking into this and doing something like:

const lighthouse = require('./node_modules/lighthouse/lighthouse-core/index');
const ExtensionProtocol = require('./node_modules/lighthouse/lighthouse-core/gather/connections/extension');

const defaultConfig = require('./node_modules/lighthouse-core/config/default-config')
const Config = require('./node_modules/lighthouse-core/config/config');
const i18n = require('./node_modules/lighthouse-core/lib/i18n/i18n.js');

And then creating a new ExtensionProtocol and calling lighthouse, the problem is that it requires my webpack to run the lighthouse build.

I'll look into using the browserified extension script - is that available as an artifact in the npm module? If it is - is there an API I can access?


update: it looks like by building the extension I can't require it in other JavaScript very easily, I can however access the globals exported by:

window.runLighthouseInExtension=runLighthouseInExtension;

window.getDefaultCategories=getDefaultCategories;

window.isRunning=isRunning;

window.listenForStatus=listenForStatus;

window.loadSettings=loadSettings;

window.saveSettings=saveSettings;

which is a start but not very customisable or great since it doesn't let me configure things very well (runLighthouseInExtension for example doesn't take a url but uses the current one in the active tab).

@benjamingr
Copy link
Author

Edit, the custom build of the browserified extension script isn't too fun to use either.

Basically what I'm requesting is an entry point I can require inside an extension (using a bundler is fine but I'd prefer not to have to roll my own brfs transform) that exposes lighthouse-core.

@benjamingr
Copy link
Author

So, doing the first idea (not using the bundle) isn't very straightforward, lighthouse requires pretty custom builds (with brfs) and getting it to work with our webpack build is pretty frustrating (brfs in webpack through transform loader didn't work well).

Currently trying to tweak the options in build-bundle with the standalone option to see if I can get meaningful output :D

@benjamingr
Copy link
Author

Would you be open to a PR adding a flag that exports an option to take a tabId and URL as a property exported from extension-entry.js?

I'm willing to contribute this if you'd take it.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Feb 28, 2019

Use @brendankenny's comment, it's much better 😄

If you're super curious about my thoughts, you can check the edit history.

@brendankenny
Copy link
Member

brendankenny commented Feb 28, 2019

hi @benjamingr!

If I understand what you're trying to do, I think this is possible, yeah.

extension-entry.js is like 95% specialized for the lighthouse extension (using chrome.storage for settings, updating the little animated lighthouse, etc), so it's probably overkill to reuse it. Instead, the main thing to reuse/adapt is ExtensionConnection in lighthouse-core/gather/connections/extension.js, and then you can just use lighthouse-core as a normal module with that connection.

I'm thinking something like this

// known-tab-extension-connection.js
const ExtensionConnection = require('./lighthouse-core/gather/connections/extension.js');

class KnownTabExtensionConnection extends ExtensionConnection {
  /** @param {number} currentTab */
  constructor(currentTab) {
    super();
    this._currentTab = currentTab;
  }

  /** @return {Promise<chrome.tabs.Tab>} */
  _queryCurrentTab() {
    return new Promise((resolve, reject) => {
      chrome.tabs.get(this._currentTab, tab => {
        if (chrome.runtime.lastError) {
          return reject(chrome.runtime.lastError);
        }

        resolve(tab);
      });
    });
  }
}

and then for the entry point

// known-tab-extension-entry.js
const lighthouse = require('./lighthouse-core/index.js');
const KnownTabExtensionConnection = require('./known-tab-extension-connection.js');
const defaultConfig = require('./lighthouse-core/config/default-config.js');

/**
 * @param {number} knownTabId
 * @param {LH.Flags} flags Lighthouse flags.
 * @return {Promise<LH.Result>}
 */
async function runLighthouse(knownTabId, flags) {
  const connection = new KnownTabExtensionConnection(knownTabId);
  const url = await connection.getCurrentTabURL();

  const runnerResult = await lighthouse(url, flags, config, connection);
  if (!runnerResult) throw new Error('did you run Lighthouse in gatherMode?');

  return runnerResult.lhr;
}

// module and/or window export of runLighthouse()

Running node ./build/build-bundle.js known-tab-extension-entry.js lighthouse-known-ext-bundle.js should give you a useable bundle (apologies for all the terrible file names :).

Some issues remain based on the particulars of what you end up needing.

  • If you want to limit the audits run in Lighthouse, just add onlyAudits or onlyCategories to the flags argument passed in
  • You could pass in the url you want to test instead of getting it from connection.getCurrentTabURL().
  • If you want to test a tab with chrome.debugger already attached, you'll need to override ExtensionConnection.connect() and ``ExtensionConnection.disconnect()` as well so it doesn't reconnect (at least, I don't know what happens if it tries to connect when already connected)
  • I'm not sure if there's much that can be done about the brfs transform, since we rely on it to inline strings that are brought in via fs.readFileSync (which obvs isn't available in an extension). Same goes for the browserify.ignore/require calls, keeping stuff out that won't work in a browser and making sure dynamically-loaded files are in the bundle. Maybe we can make this easier to consume, though?
  • Lighthouse is written assuming that it's the only thing messing with the page via the debugger protocol. If that's not true (e.g. some override settings are already set) there may be unexpected interactions or it may be totally fine. We'll have to see what happens :)

@benjamingr
Copy link
Author

Thanks! I actually did get this to work by keeping the existing build, editing runLighthouseInExtension in the source and passing it the tabId manually for the connection.

I actually started with just using lighthouse-core and ExtensionConnetion but that ended up being hard because of the particular transforms and build lighthouse uses for building for extensions (like brfs, ignoring a bunch of folders etc).

I talked to @pkrumins and he told me we can do something like adding a third build option to lighthouse that sets standalone: 'lighthouse' as a build option that could export lighthouse.

Alternatively - we can add a build flag that doesn't export all the current window globals but instead exports something an extension can use like a method taking a tabId, url and what transforms to use and runs lighthouse. We can export that to NPM and have that consumable by extensions - what do you think about that option?

It'd really be great not having to keep a fork of the code for this and using lighthouse as an npm module for browser extension development.

@brendankenny
Copy link
Member

brendankenny commented Mar 25, 2019

It'd really be great not having to keep a fork of the code for this and using lighthouse as an npm module for browser extension development.

Rather than publishing another npm module (with the maintenance and ownership overhead that brings), I think maybe the effort could be put into making the use of lighthouse as an npm module in this way easier.

A Connection is able to be passed into the lighthouse module specifically for allowing use cases like this without requiring users to fork lighthouse. Maybe we could do whatever work is needed to better clean and document the Driver <-> Connection interface so this is easier and more obvious to do. That would improve the core of lighthouse and allow easier downstream usage.

Then it sounds like adding a --standalone (or whatever) flag to ./build/build-bundle.js would let you consume it in the browser (extension) space without maintaining your parallel browserify pipeline. The main thing we would want to do is automate maintenance (and not breaking users) as much as possible, so some good tests of the bundled output would be necessary.

@brendankenny
Copy link
Member

The main thing we would want to do is automate maintenance (and not breaking users) as much as possible, so some good tests of the bundled output would be necessary.

maybe we could even always do standalone: 'lighthouse' and move the lighthouse (core) extension to using it. Then the existing test pipeline would make sure things are still working.

We'd have to see if that would be a net improvement in code readability or if it would just make things more complicated than it's worth, though.

@benjamingr
Copy link
Author

That would be awesome @brendankenny

@brendankenny
Copy link
Member

The extension has been increasingly difficult to maintain due to changes in the protocol, limitations of the protocol through the extension APIs, and increased out-of-process use in Chrome, so we've decided to remove the actual run of Lighthouse in the extension. Instead, users will be pointed to PSI and DevTools. As a result, unfortunately we won't be moving forward with this and will be removing extension-specific code.

See #9193 for progress on that.

@benjamingr
Copy link
Author

benjamingr commented Sep 20, 2019

I am confused - what was hard about maintaining extensions in the devtools (the protocol rarely changes nowadays anyway?).

Also - what do you mean by PSI? does that mean users will be referred to a google hosted service and that the devtools will send site content to google servers rather than run audits locally? Or do you mean users will still be able to run audits locally but that will require node and PSI will be offered as a free alternative for users who do not want to run node?

@brendankenny
Copy link
Member

I am confused - what was hard about maintaining extensions in the devtools (the protocol rarely changes nowadays anyway?).

See discussion in #8690 and things like the saga of trying to get OOPIF support in #7517, #7566, and finally #7608.

Also - what do you mean by PSI? ...do you mean users will still be able to run audits locally but that will require node and PSI will be offered as a free alternative for users who do not want to run node?

Exactly this. The extension will offer PSI as an option for users still wanting a one-click Lighthouse run (with clear indication that it uses an API for the service) as well as offering the option of opening DevTools and running it there. DevTools and the node versions of Lighthouse will not be changed, still run completely locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants