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(fr): add base config #11915

Merged
merged 1 commit into from
Jan 8, 2021
Merged

core(fr): add base config #11915

merged 1 commit into from
Jan 8, 2021

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jan 5, 2021

Summary
Base config logic for the new Fraggle Rock config. It's lots of lines, but mostly a move + adding missing tests.

  • Moves shared logic from config.js to config-helpers.js
    • merge logic
    • settings resolution and validation
    • gatherer resolution and validation
  • Unites some naming and structural discrepancies between the two configs and gatherers/audits
    • merge was fairly generic when it has some highly specific config logic it it, renamed to mergeConfigFragment
    • resolveThing is the new nomenclature used for converting from config input to config output (i.e. thingJson to thingDefn). This was previously some mixture of requireThing/initializeThing
    • requireThing is now just the logic to actually require an individual Thing
  • Creates a new fraggle-rock/config/config.js to contain new config logic.
    • The barebones artifact flow for now. Remaining logic is stubbed out in tests and comments.
  • Adds tests for previously untested config helpers.

Related Issues/PRs
ref #11313
Design Doc

* @param {Object<string, any>|Array<any>} extension
* @param {boolean=} overwriteArrays
*/
function _mergeConfigFragment(base, extension, overwriteArrays = false) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

logic here was untouched but comment and name are new

function expandAuditShorthand(audits) {
if (!audits) {
return null;
function expandAuditShorthand(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.

changed to operate on an individual audit instead of an array for consistency with gatherers, otherwise untouched

} else {
// See if the audit is a Lighthouse core audit.
const auditPathJs = `${audit.path}.js`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

extracted this into requireAudit for structural consistency with resolveGathererToDefn

function requireAudits(audits, configDir) {
const expandedAudits = expandAuditShorthand(audits);
if (!expandedAudits) {
function resolveAuditsToDefns(audits, configDir) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I stopped short of converting this one to individual because of the minor annoyance of the Runner.getAuditList call, but if everyone would prefer that final step for more complete consistency, I'm game

@@ -0,0 +1,107 @@
/**
* @license Copyright 2020 The Lighthouse Authors. All Rights Reserved.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did write this file in 2020, before anyone asks :P

Copy link
Contributor

@Beytoven Beytoven left a comment

Choose a reason for hiding this comment

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

LGTM!

@patrickhulce patrickhulce merged commit 826301e into master Jan 8, 2021
@patrickhulce patrickhulce deleted the fr_config_base branch January 8, 2021 15:25
paulirish pushed a commit that referenced this pull request Mar 23, 2021
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