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): refactor config cloning for fraggle rock #11759

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

patrickhulce
Copy link
Collaborator

Summary
This PR is the first baby step toward the fraggle rock config.

  • Split out the cloning into config-helpers
  • Add tests for these currently untested functions
  • Add type defs to differentiate FR config from current config and the ArtifactDefn concept
  • Add cloning support for the artifacts config property

Related Issues/PRs
ref #11313 #11748

@patrickhulce patrickhulce requested a review from a team as a code owner December 3, 2020 19:16
@patrickhulce patrickhulce requested review from Beytoven and removed request for a team December 3, 2020 19:16
@google-cla google-cla bot added the cla: yes label Dec 3, 2020
* @param {T} item
* @return {T}
*/
function shallowClone(item) {
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 function used to be called cloneArrayWithPluginSafety (we used to call the custom audit/gatherer definition "plugin support") which became very misleading once we actually introduced plugin support that means something else. This is just more straightforward for what it was.

@@ -1,3 +1,4 @@
/* eslint-disable strict */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is anyone else having this problem in vscode where eslint is marking 100% of our .d.ts files as failing because of the strictness? started happening for me once we switched to the typescript parser in eslint

Copy link
Member

Choose a reason for hiding this comment

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

yup same. vscode's really unhappy with my .d.ts files.

expect(input.categories.random.auditRefs[0].id).toEqual('user-timings');
});

it('should preserve gatherer implementations in passes', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Perhaps it's cleaner this way and it doesn't really matter, but it seems a bit unnecessary for these to be separate cases. We could just have a "preserves implementations" test that covers all three cases as they are all testing the same function and behavior.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

cool

@@ -1,3 +1,4 @@
/* eslint-disable strict */
Copy link
Member

Choose a reason for hiding this comment

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

yup same. vscode's really unhappy with my .d.ts files.

@@ -16,13 +17,36 @@ declare global {
export interface Json {
extends?: 'lighthouse:default' | string | boolean;
settings?: SharedFlagsSettings;
artifacts?: ArtifactJson[] | null;
Copy link
Member

Choose a reason for hiding this comment

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

ah that's pleasant.

I faintly remember some other hack where we allowed something to be expressed in JSON instead of read from a filepath. Ah, looks like it was --extra-headers.

nevermind carry on!

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.

4 participants