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: deprecate passes, remove config navigations from FR #13881

Merged
merged 25 commits into from
Aug 24, 2022

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Apr 20, 2022

As discussed here, this PR removes config navigations (known as passes in legacy mode) from FR navigations.

There are some settings which are only configurable on a legacy pass/config navigation (e.g. pauseAfterFCPMs). I think it's better to define these on config.settings rather than maintain them in a half-deprecated state on config.passes.

Comment on lines 108 to 101
id: string;
/** Whether throttling settings should be skipped for the pass. */
disableThrottling?: boolean;
/** Whether storage clearing (service workers, cache storage) should be skipped for the pass. A run-wide setting of `true` takes precedence over this value. */
disableStorageReset?: boolean;
/** The array of artifacts to collect during the navigation. */
artifacts?: Array<string>;
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't add any of these settings to config.settings because:

  • disableStorageReset is already on config.settings
  • id doesn't make sense if there is only one navigation
  • disableThrottling can be achieved with config.settings.throttlingMethod = 'provided'
  • config.artifacts is now the canonical list of artifacts, we don't need to maintain a second one

@@ -98,7 +98,6 @@ const defaultConfig = {
{id: artifacts.EmbeddedContent, gatherer: 'seo/embedded-content'},
{id: artifacts.FontSize, gatherer: 'seo/font-size'},
{id: artifacts.Inputs, gatherer: 'inputs'},
{id: artifacts.FullPageScreenshot, gatherer: 'full-page-screenshot'},
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 actually a minor bug fix for timespan/snapshot because they weren't doing FPS as the final gatherer before.

@brendankenny
Copy link
Member

brendankenny commented Apr 20, 2022

This has more changes than I was picturing. I thought we were talking about moving the new default-config to basically the form you have it in this PR, but then config.js would internally convert it to the form it has today (creating a navigations array (or rename it passes), etc).

That would allow basically everything else to remain as it is today. More stuff to maintain, but fewer additional changes to get right before 10.0, leaves the door open to changing our mind on ditching passes, smoke tests still work, etc.

@adamraine
Copy link
Member Author

I thought we were talking about moving the new default-config to basically the form you have it in this PR, but then config.js would internally convert it to the form it has today (creating a navigations array (or rename it passes), etc).

That's what I was looking into originally, the problem is what to do with the extra settings defined on each pass. It would leave passes in a weird half-deprecated state where we don't want users to use it with FR navigations but some of the settings still matter.

@brendankenny
Copy link
Member

That's what I was looking into originally, the problem is what to do with the extra settings defined on each pass.

Can you give more specifics? Since it's only the config input, it seems like it wouldn't matter.

  • If you use the new config format, it only supports the single navigation, so copying from the settings to (internally) create settings for a pass seems like it would be unambiguous.
  • If you use the old config format, nothing changes

It would leave passes in a weird half-deprecated state where we don't want users to use it with FR navigations but some of the settings still matter.

Yeah, but passes will be in a weird half-deprecated state so that just matches what we're doing :)

@adamraine
Copy link
Member Author

Can you give more specifics? Since it's only the config input, it seems like it wouldn't matter.

We would be telling users that passes are deprecated for the new config format, but if they want to configure something like cpuQuietThresholdMs then they would have to modify the passes on the config.

@brendankenny
Copy link
Member

We would be telling users that passes are deprecated for the new config format, but if they want to configure something like cpuQuietThresholdMs then they would have to modify the passes on the config.

Oh, I thought we were discussing removing passes completely on the new config format, not just deprecating it. If someone needs passes they can use the deprecated classic config format for that. If they update to the new format cpuQuietThresholdMs would go on settings.

@adamraine
Copy link
Member Author

Oh, I thought we were discussing removing passes completely on the new config format, not just deprecating it. If someone needs passes they can use the deprecated classic config format for that. If they update to the new format cpuQuietThresholdMs would go on settings.

Ok I think I was confused and got turned around. You don't have an issue with adding the pass settings to config.settings, you just want to create a fake internal navigation so we don't have to change everything. I think I'm on board with that.

@adamraine
Copy link
Member Author

@brendankenny @connorjclark this should be ready for review

@adamraine adamraine added the 10.0 label Aug 22, 2022
core/test/fraggle-rock/gather/navigation-runner-test.js Outdated Show resolved Hide resolved
core/test/fraggle-rock/gather/navigation-runner-test.js Outdated Show resolved Hide resolved
core/test/fraggle-rock/gather/navigation-runner-test.js Outdated Show resolved Hide resolved
core/test/fraggle-rock/gather/navigation-runner-test.js Outdated Show resolved Hide resolved
core/config/constants.js Outdated Show resolved Hide resolved
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