-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
core: initial refactor of computedArtifact import/caching #5907
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
*/ | ||
'use strict'; | ||
|
||
const ComputedArtifact = require('./computed-artifact'); | ||
const makeComputedArtifact = require('./new-computed-artifact'); | ||
const icons = require('../../lib/icons'); | ||
|
||
const PWA_DISPLAY_VALUES = ['minimal-ui', 'fullscreen', 'standalone']; | ||
|
@@ -14,11 +14,7 @@ const PWA_DISPLAY_VALUES = ['minimal-ui', 'fullscreen', 'standalone']; | |
// For more discussion, see https://github.com/GoogleChrome/lighthouse/issues/69 and https://developer.chrome.com/apps/manifest/name#short_name | ||
const SUGGESTED_SHORTNAME_LENGTH = 12; | ||
|
||
class ManifestValues extends ComputedArtifact { | ||
get name() { | ||
return 'ManifestValues'; | ||
} | ||
|
||
class ManifestValues { | ||
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. I believe someone once told me that he thought it was silly we made classes with all static members instead of plain objects ;) FWIW I kinda like them as classes for the flashy 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.
haha, I didn't even think about it as I changed things. There used to be some tsc benefit due to polymorphic I'll admit to stockholm syndrome in that I don't see it as ugly and unnecessary any more, but I also don't have any problem with a plain object. The main thing might be the 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. true true, this WFM :) |
||
static get validityIds() { | ||
return ['hasManifest', 'hasParseableManifest']; | ||
} | ||
|
@@ -88,7 +84,7 @@ class ManifestValues extends ComputedArtifact { | |
* @param {LH.Artifacts['Manifest']} manifest | ||
* @return {Promise<LH.Artifacts.ManifestValues>} | ||
*/ | ||
async compute_(manifest) { | ||
static async compute_(manifest) { | ||
// if the manifest isn't there or is invalid json, we report that and bail | ||
let parseFailureReason; | ||
|
||
|
@@ -125,4 +121,4 @@ class ManifestValues extends ComputedArtifact { | |
} | ||
} | ||
|
||
module.exports = ManifestValues; | ||
module.exports = makeComputedArtifact(ManifestValues); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
/** | ||
* @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 ArbitraryEqualityMap = require('../../lib/arbitrary-equality-map.js'); | ||
|
||
/** | ||
* @template {string} N | ||
* @template A | ||
* @template R | ||
* @param {{name: N, compute_: (artifact: A) => Promise<R>}} computableArtifact | ||
* @return {{name: N, request: (context: LH.Audit.Context, artifact: A) => Promise<R>}} | ||
*/ | ||
function makeComputedArtifact(computableArtifact) { | ||
/** | ||
* @param {LH.Audit.Context} context | ||
* @param {A} artifact | ||
*/ | ||
const request = ({computedCache}, artifact) => { | ||
const cache = computedCache.get(computableArtifact.name) || new ArbitraryEqualityMap(); | ||
computedCache.set(computableArtifact.name, cache); | ||
|
||
const computed = /** @type {Promise<R>|undefined} */ (cache.get(artifact)); | ||
if (computed) { | ||
return computed; | ||
} | ||
|
||
const artifactPromise = computableArtifact.compute_(artifact); | ||
cache.set(artifact, artifactPromise); | ||
|
||
return artifactPromise; | ||
}; | ||
|
||
return Object.assign(computableArtifact, {request}); | ||
} | ||
|
||
module.exports = makeComputedArtifact; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,10 +205,17 @@ class Runner { | |
} | ||
} | ||
|
||
// Members of LH.Audit.Context that are shared across all audits. | ||
const sharedAuditContext = { | ||
settings, | ||
LighthouseRunWarnings: runWarnings, | ||
computedCache: new Map(), | ||
}; | ||
|
||
// Run each audit sequentially | ||
const auditResults = []; | ||
for (const auditDefn of audits) { | ||
const auditResult = await Runner._runAudit(auditDefn, artifacts, settings, runWarnings); | ||
const auditResult = await Runner._runAudit(auditDefn, artifacts, sharedAuditContext); | ||
auditResults.push(auditResult); | ||
} | ||
|
||
|
@@ -220,12 +227,11 @@ class Runner { | |
* Otherwise returns error audit result. | ||
* @param {LH.Config.AuditDefn} auditDefn | ||
* @param {LH.Artifacts} artifacts | ||
* @param {LH.Config.Settings} settings | ||
* @param {Array<string>} runWarnings | ||
* @param {Pick<LH.Audit.Context, 'settings'|'LighthouseRunWarnings'|'computedCache'>} sharedAuditContext | ||
* @return {Promise<LH.Audit.Result>} | ||
* @private | ||
*/ | ||
static async _runAudit(auditDefn, artifacts, settings, runWarnings) { | ||
static async _runAudit(auditDefn, artifacts, sharedAuditContext) { | ||
const audit = auditDefn.implementation; | ||
const status = `Evaluating: ${i18n.getFormatted(audit.meta.title, 'en-US')}`; | ||
|
||
|
@@ -274,8 +280,7 @@ class Runner { | |
const auditOptions = Object.assign({}, audit.defaultOptions, auditDefn.options); | ||
const auditContext = { | ||
options: auditOptions, | ||
settings, | ||
LighthouseRunWarnings: runWarnings, | ||
...sharedAuditContext, | ||
}; | ||
|
||
const product = await audit.audit(artifacts, auditContext); | ||
|
@@ -380,6 +385,10 @@ class Runner { | |
'metrics', // the sub folder that contains metric names | ||
'metrics/lantern-metric.js', // lantern metric base class | ||
'metrics/metric.js', // computed metric base class | ||
|
||
// Computed artifacts switching to the new system. | ||
'new-computed-artifact.js', | ||
'manifest-values.js', | ||
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 fn is for the gulpfile. wont browserify already discover this manifest-values module without being added manually? 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. the diff obscures it, but this is the list of filenamesToSkip :) |
||
]; | ||
|
||
const fileList = [ | ||
|
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'll refrain from going promises->async/await in future changes to cut down the diffs as well :)