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

report(links): utm params for all developer.google.com links #4972

Closed
wants to merge 12 commits into from
1 change: 1 addition & 0 deletions lighthouse-cli/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@
*/
'use strict';

process.env.LH_CHANNEL = process.env.LH_CHANNEL || 'cli';
require('./bin.js').run();
22 changes: 19 additions & 3 deletions lighthouse-core/report/html/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ class CategoryRenderer {
this.detailsRenderer = detailsRenderer;
/** @type {ParentNode} */
this.templateContext = this.dom.document();
/** @protected {?string} */
this.reportChannel = null;

this.detailsRenderer.setTemplateContext(this.templateContext);
}
Expand Down Expand Up @@ -69,7 +71,10 @@ class CategoryRenderer {
const titleEl = this.dom.find('.lh-audit__title', auditEl);
titleEl.appendChild(this.dom.convertMarkdownCodeSnippets(audit.result.title));
this.dom.find('.lh-audit__description', auditEl)
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description, {
channel: this.reportChannel,
rating: Util.calculateRating(audit.result.score, scoreDisplayMode),
}));

const header = /** @type {HTMLDetailsElement} */ (this.dom.find('details', auditEl));
if (audit.result.details && audit.result.details.type) {
Expand Down Expand Up @@ -147,7 +152,9 @@ class CategoryRenderer {
this.dom.find('.lh-category-header__title', tmpl).appendChild(
this.dom.convertMarkdownCodeSnippets(category.title));
if (category.description) {
const descEl = this.dom.convertMarkdownLinkSnippets(category.description);
const descEl = this.dom.convertMarkdownLinkSnippets(category.description, {
channel: this.reportChannel,
});
this.dom.find('.lh-category-header__description', tmpl).appendChild(descEl);
}

Expand All @@ -174,7 +181,9 @@ class CategoryRenderer {

if (group.description) {
const auditGroupDescription = this.dom.createElement('div', 'lh-audit-group__description');
auditGroupDescription.appendChild(this.dom.convertMarkdownLinkSnippets(group.description));
auditGroupDescription.appendChild(this.dom.convertMarkdownLinkSnippets(group.description, {
channel: this.reportChannel,
}));
groupEl.appendChild(auditGroupDescription);
}
headerEl.textContent = group.title;
Expand Down Expand Up @@ -230,6 +239,13 @@ class CategoryRenderer {
return passedElem;
}

/**
* @param {string} channel
*/
setReportChannel(channel) {
this.reportChannel = channel;
}

/**
* @param {Array<Element>} elements
* @return {Element}
Expand Down
21 changes: 19 additions & 2 deletions lighthouse-core/report/html/renderer/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,17 @@ class DOM {

/**
* @param {string} text
* @param {{channel?: string | null, rating?: string}=} metadata
* @return {Element}
*/
convertMarkdownLinkSnippets(text) {
convertMarkdownLinkSnippets(text, metadata) {
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
const element = this.createElement('span');

// Split on markdown links (e.g. [some link](https://...)).
const parts = text.split(/\[([^\]]*?)\]\((https?:\/\/.*?)\)/g);

const DEVELOPERS_GOOGLE_ORIGIN = 'https://developers.google.com';

while (parts.length) {
// Pop off the same number of elements as there are capture groups.
const [preambleText, linkText, linkHref] = parts.splice(0, 3);
Expand All @@ -126,7 +129,21 @@ class DOM {
a.rel = 'noopener';
a.target = '_blank';
a.textContent = linkText;
a.href = (new URL(linkHref)).href;

const url = new URL(linkHref);

if (url.origin === DEVELOPERS_GOOGLE_ORIGIN) {
Anthony5535 marked this conversation as resolved.
Show resolved Hide resolved
url.searchParams.set('utm_source', 'lighthouse');

if (metadata && metadata.channel) {
url.searchParams.set('utm_medium', metadata.channel);
}
if (metadata && metadata.rating) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh my, I'm really regretting us calling that pass/fail string rating now 😆

url.searchParams.set('utm_content', metadata.rating);
}
}

a.href = url.href;
element.appendChild(a);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
valueEl.textContent = Util.formatDisplayValue(audit.result.displayValue);

const descriptionEl = this.dom.find('.lh-metric__description', tmpl);
descriptionEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
descriptionEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description, {
channel: this.reportChannel,
}));

if (audit.result.scoreDisplayMode === 'error') {
descriptionEl.textContent = '';
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/report/html/renderer/report-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ class ReportRenderer {
const perfCategoryRenderer = new PerformanceCategoryRenderer(this._dom, detailsRenderer);
perfCategoryRenderer.setTemplateContext(this._templateContext);

if (report.lighthouseVersion.includes('+')) {
const lighthouseChannel = report.lighthouseVersion.split('+')[1];
categoryRenderer.setReportChannel(lighthouseChannel);
perfCategoryRenderer.setReportChannel(lighthouseChannel);
}

const categories = reportSection.appendChild(this._dom.createElement('div', 'lh-categories'));

for (const category of report.reportCategories) {
Expand Down
7 changes: 6 additions & 1 deletion lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,12 @@ class Runner {
}

// Entering: conclusion of the lighthouse result object
const lighthouseVersion = require('../package.json').version;

// @ts-ignore - Needs json require() support
Copy link
Collaborator

Choose a reason for hiding this comment

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

@brendankenny do you have any tips for him here?

not sure why this would be be necessary now when it wasn't before 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think this is just a bad merge or rebase. I just removed this ts-ignore in the last week or two

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, I removed it 👍

const lighthousePackageVersion = require('../package.json').version;
const lighthouseChannel = process.env.LH_CHANNEL || 'nm';

const lighthouseVersion = `${lighthousePackageVersion}+${lighthouseChannel}`;

/** @type {Object<string, LH.Audit.Result>} */
const resultsById = {};
Expand Down
18 changes: 18 additions & 0 deletions lighthouse-core/test/report/html/renderer/dom-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,24 @@ describe('DOM', () => {
assert.equal(result.innerHTML, 'Ensuring `&lt;td&gt;` cells using the `[headers]` are ' +
'good. <a rel="noopener" target="_blank" href="https://dequeuniversity.com/rules/axe/2.2/td-headers-attr">Learn more</a>.');
});

it('appends utm params to the URLs with https://developers.google.com origin', () => {
const text = '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/description).';

let result = dom.convertMarkdownLinkSnippets(text);
assert.equal(result.innerHTML, '<a rel="noopener" target="_blank" href="https://developers.google.com/web/tools/lighthouse/audits/description?utm_source=lighthouse">Learn more</a>.');

result = dom.convertMarkdownLinkSnippets(text, {
channel: 'cli',
});
assert.equal(result.innerHTML, '<a rel="noopener" target="_blank" href="https://developers.google.com/web/tools/lighthouse/audits/description?utm_source=lighthouse&amp;utm_medium=cli">Learn more</a>.');

result = dom.convertMarkdownLinkSnippets(text, {
channel: 'ext',
rating: 'fail',
});
assert.equal(result.innerHTML, '<a rel="noopener" target="_blank" href="https://developers.google.com/web/tools/lighthouse/audits/description?utm_source=lighthouse&amp;utm_medium=ext&amp;utm_content=fail">Learn more</a>.');
});
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
});

describe('convertMarkdownCodeSnippets', () => {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"hostUserAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3358.0 Safari/537.36",
"benchmarkIndex": 1000
},
"lighthouseVersion": "3.0.3",
"lighthouseVersion": "3.0.3+cli",
"fetchTime": "2018-03-13T00:55:45.840Z",
"requestedUrl": "http://localhost:10200/dobetterweb/dbw_tester.html",
"finalUrl": "http://localhost:10200/dobetterweb/dbw_tester.html",
Expand Down Expand Up @@ -3793,4 +3793,4 @@
]
}
}
}
}
9 changes: 6 additions & 3 deletions lighthouse-extension/gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,11 @@ gulp.task('chromeManifest', () => {
.pipe(gulp.dest(distDir));
});

function applyBrowserifyTransforms(bundle) {
function applyBrowserifyTransforms(bundle, channel) {
// Fix an issue with imported speedline code that doesn't brfs well.
return bundle.transform('./fs-transform', {global: true})
// Replace LH_CHANNEL environment variable with a string
.transform('./lh-channel-transform', {channel, global: true})
Copy link
Collaborator Author

@kdzwinel kdzwinel Jul 26, 2018

Choose a reason for hiding this comment

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

This is a custom find&replace transform. I couldn't use envify because it's based on esprima 4 (latest) which doesn't support object rest/spread :(

// Transform the fs.readFile etc, but do so in all the modules.
.transform('brfs', {global: true, parserOpts: {ecmaVersion: 9}})
// Strip everything out of package.json includes except for the version.
Expand All @@ -119,8 +121,9 @@ gulp.task('browserify-lighthouse', () => {
'app/src/lighthouse-ext-background.js',
], {read: false})
.pipe(tap(file => {
const channel = /lighthouse-background/.test(file.path) ? 'cdt' : 'ext';
let bundle = browserify(file.path); // , {debug: true}); // for sourcemaps
bundle = applyBrowserifyTransforms(bundle);
bundle = applyBrowserifyTransforms(bundle, channel);

// scripts will need some additional transforms, ignores and requires…
bundle.ignore('source-map')
Expand All @@ -135,7 +138,7 @@ gulp.task('browserify-lighthouse', () => {
bundle.ignore(require.resolve('../lighthouse-core/gather/connections/cri.js'));

// Prevent the DevTools background script from getting the stringified HTML.
if (/lighthouse-background/.test(file.path)) {
if (channel === 'cdt') {
bundle.ignore(require.resolve('../lighthouse-core/report/html/html-report-assets.js'));
}

Expand Down
31 changes: 31 additions & 0 deletions lighthouse-extension/lh-channel-transform.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* @license Copyright 2018 Google Inc. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* 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.
*/
'use strict';

const through = require('through2');

/**
* This is a browserify transform replaces LH_CHANNEL node environment variable
* It should be replaced with envify as soon as it supports object rest/spread properties
*/
module.exports = function(file, options) {
const fileContents = [];
return through(function(part, enc, next) {
fileContents.push(part);
next();
}, function(done) {
let fileContentsString = fileContents.join('');
const needle = 'process.env.LH_CHANNEL';

if (fileContentsString.includes(needle)) {
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
fileContentsString = fileContentsString.replace(needle, `'${options.channel}'`);
mattzeunert marked this conversation as resolved.
Show resolved Hide resolved
}

// eslint-disable-next-line no-invalid-this
this.push(fileContentsString);
done();
});
};
2 changes: 1 addition & 1 deletion lighthouse-viewer/app/src/lighthouse-report-viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ class LighthouseReportViewer {
}

// Leave off patch version in the comparison.
const semverRe = new RegExp(/^(\d+)?\.(\d+)?\.(\d+)$/);
const semverRe = new RegExp(/^(\d+)?\.(\d+)?\.(\d+)(\+[a-z]+)?$/);
const reportVersion = reportJson.lighthouseVersion.replace(semverRe, '$1.$2');
const lhVersion = window.LH_CURRENT_VERSION.replace(semverRe, '$1.$2');

Expand Down