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

core(config): add dedicated ID to audit/gatherer defns #8374

Closed
wants to merge 4 commits into from

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Apr 17, 2019

Summary
So blessed artifacts turned out to be quite the rabbit hole. This is part 1 of 3 stages.

  1. (This PR) Split apart some config logic into a helper file and add a dedicated ID to audit and gatherer definitions so that they are more easily referenceable. This also has the happy side effect of enabling the use of options in plugin audits in step 2 :D
  2. Enforce that every plugin audit id is prefixed with <plugin-id>. It will still be able to reference our existing core audit implementations.
  3. Load each audit when loading a plugin to check its requiredArtifacts against a set of blessed artifacts

Related Issues/PRs
#6747

Co-Authored-By: patrickhulce <patrick.hulce@gmail.com>
@paulirish
Copy link
Member

Split apart some config logic into a helper file and add a dedicated ID to audit and gatherer definitions so that they are more easily referenceable.

can you point to the second part?

looks like these share a commit so its a tad harder to review.

}

return {
id: audit.id || (implementation.meta && implementation.meta.id),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this line is new

throw new Error(`${auditName} has no audit() method.`);
}

if (typeof id !== 'string' || !id.trim()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this if is new

*/
function assertValidAudit(auditDefinition) {
const {id, implementation, path: auditPath} = auditDefinition;
const auditName = id || auditPath || 'Unknown audit';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these two lines at the top are new to use id over auditPath when available since path could now be ambiguous

const mergedItems = [];

for (const item of items) {
const existingItem = mergedItems.find(candidate => candidate.id === item.id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this now uses id instead of path

@patrickhulce
Copy link
Collaborator Author

looks like these share a commit so its a tad harder to review.

yeah sorry if I would've thought about it I would commit stopped :(

* @param {LH.Config.Json['audits']} audits
* @return {?Array<{id?: string, path: string, options?: {}} | {id?: string, implementation: typeof Audit, path?: string, options?: {}}>}
*/
function expandAuditShorthand(audits) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no changes here

* @return {string}
* @throws {Error}
*/
function resolveModule(moduleIdentifier, configDir, category) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no changes here

@patrickhulce
Copy link
Collaborator Author

hang on for reviewing Imma split this

@patrickhulce
Copy link
Collaborator Author

this needs to be reworked given our new direction with plugins

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

Successfully merging this pull request may close these issues.

3 participants