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

core(tsc): add type checking to runner #4961

Merged
merged 5 commits into from
Apr 13, 2018
Merged

core(tsc): add type checking to runner #4961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
203a584
core(tsc): add type checking to runner
brendankenny Apr 12, 2018
1834964
lint and add test for notApplicable weighting
brendankenny Apr 13, 2018
da5f9e6
feedback
brendankenny Apr 13, 2018
d3ba187
add type TODO
brendankenny Apr 13, 2018
5cbd595
throw the error directly
brendankenny Apr 13, 2018
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
14 changes: 14 additions & 0 deletions lighthouse-core/audits/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@ class Audit {
return {};
}

/* eslint-disable no-unused-vars */

/**
*
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {LH.Audit.Product|Promise<LH.Audit.Product>}
*/
static audit(artifacts, context) {
throw new Error('audit() method must be overriden');
}

/* eslint-enable no-unused-vars */

/**
* Computes a clamped score between 0 and 1 based on the measured value. Score is determined by
* considering a log-normal distribution governed by the two control points, point of diminishing
Expand Down
7 changes: 6 additions & 1 deletion lighthouse-core/audits/seo/robots-txt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class RobotsTxt extends Audit {
}

/**
* @param {{RobotsTxt: {status: number, content: string}}} artifacts
* @param {LH.Artifacts} artifacts
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
Expand Down Expand Up @@ -198,6 +198,11 @@ class RobotsTxt extends Audit {
};
}

// If status is good, content must be not null.
if (content === null) {
throw new Error(`Status ${status} was valid, but content was null`);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@kdzwinel had to add this to keep the type checker happy even though it appears to not be possible. I couldn't think of a good way around this short of adding another property that lets you demonstrate to the type checker that a null content can't happen in certain cases, e.g.
{error: false, status: number, content: string}|{error: true, status: number|null, content: string|null}
...or we can just test like here for // can't ever happen :)


const validationErrors = validateRobots(content);

const headings = [
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/config/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ function assertValidAudit(auditDefinition, auditPath) {
const auditName = auditPath ||
(auditDefinition && auditDefinition.meta && auditDefinition.meta.name);

if (typeof auditDefinition.audit !== 'function') {
if (typeof auditDefinition.audit !== 'function' || auditDefinition.audit === Audit.audit) {
throw new Error(`${auditName} has no audit() method.`);
}

Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/lib/asset-saver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,13 @@ async function prepareAssets(artifacts, audits) {
for (const passName of passNames) {
const trace = artifacts.traces[passName];
const devtoolsLog = artifacts.devtoolsLogs[passName];

// Avoid Runner->AssetSaver->Runner circular require by loading Runner here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

:(

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, really disappointed to get there and run into that. Hopefully with computedArtifacts refactor that'll get cleaned up

const Runner = require('../runner.js');
const computedArtifacts = Runner.instantiateComputedArtifacts();
/** @type {Array<Screenshot>} */
// @ts-ignore TODO(bckenny): need typed computed artifacts
const screenshots = await artifacts.requestScreenshots(trace);
const screenshots = await computedArtifacts.requestScreenshots(trace);

const traceData = Object.assign({}, trace);
const screenshotsHTML = screenshotDump(screenshots);
Expand Down
Loading