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): replace configContext with flags #14050

Merged
merged 16 commits into from
Aug 16, 2022
Merged

Conversation

adamraine
Copy link
Member

First item in #14049

@@ -118,6 +118,8 @@ export interface Flags extends SharedFlagsSettings {
configPath?: string;
/** Run the specified plugins. */
plugins?: string[];
/** If set to true, will skip the initial navigation to about:blank. This option is ignored when using the legacy navigation runner. */
skipAboutBlank?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only option in the FR context that isn't present on the legacy flags. @paulirish I remember when we added this you were concerned about exposing it to the user, but I'm not sure how else to do it without creating another options object for internal stuff.

Copy link
Member

Choose a reason for hiding this comment

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

All good. I'm fine with it moving here

Copy link
Member

Choose a reason for hiding this comment

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

if skipAboutBlank is going to be exposed, it should just be a setting (and put on SharedFlagsSettings). It makes more sense as a config-level setting than a module-level one anyways.

That shouldn't change much about the implementation here, though it'll need a default in constants.js, etc. The internalOptions stuff could then be deleted, too.

@@ -118,6 +118,8 @@ export interface Flags extends SharedFlagsSettings {
configPath?: string;
/** Run the specified plugins. */
plugins?: string[];
/** If set to true, will skip the initial navigation to about:blank. This option is ignored when using the legacy navigation runner. */
skipAboutBlank?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

All good. I'm fine with it moving here

lighthouse-core/fraggle-rock/config/config.js Outdated Show resolved Hide resolved
@adamraine
Copy link
Member Author

copybara-service bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this pull request Jul 19, 2022
Without this change, the following PR would break our flags usage:
GoogleChrome/lighthouse#14050

Bug: None
Change-Id: If5d05982fcd6a4fdd379e2de1406b976338cdf56
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3774533
Reviewed-by: Paul Irish <paulirish@chromium.org>
Commit-Queue: Paul Irish <paulirish@chromium.org>
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM with a skipAboutBlank type move.

@@ -118,6 +118,8 @@ export interface Flags extends SharedFlagsSettings {
configPath?: string;
/** Run the specified plugins. */
plugins?: string[];
/** If set to true, will skip the initial navigation to about:blank. This option is ignored when using the legacy navigation runner. */
skipAboutBlank?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

if skipAboutBlank is going to be exposed, it should just be a setting (and put on SharedFlagsSettings). It makes more sense as a config-level setting than a module-level one anyways.

That shouldn't change much about the implementation here, though it'll need a default in constants.js, etc. The internalOptions stuff could then be deleted, too.

@adamraine adamraine merged commit 0786898 into master Aug 16, 2022
@adamraine adamraine deleted the fr-context-to-flags branch August 16, 2022 21:33
alexnj pushed a commit to alexnj/lighthouse that referenced this pull request Aug 24, 2022
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.

None yet

4 participants