Skip to content

Conversation

@westonruter
Copy link
Collaborator

The benchmark-web-vitals command has only been testing a desktop-ish sized viewport at 960x700. In order to test a mobile viewport or a more common desktop viewport, this PR introduces a --window-viewport (-w) parameter which allows the desired window dimensions to be supplied like 360x800. The default remains 960x700.

Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@westonruter Awesome, I can't believe we didn't have something like this before - so were always only testing with this "small desktop"-ish viewport?! Wow.

One recommendation for enabling simpler usage rather than having to specify specific widths, but otherwise this is good.

showVariance: Boolean( opt.showVariance ),
cpuThrottleFactor: null,
networkConditions: null,
windowViewport: { width: 960, height: 700 }, // Viewport similar to @wordpress/e2e-test-utils 'large' configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes sense to have this parameter, but I wonder whether we should additionally have more than just this one default, to simplify usage.

I'm thinking about our typical mobile vs desktop differentiation. Maybe we could allow passing special string values mobile and desktop for the --window-viewport argument, and in that case rely on a specific default for that viewport type that we set?

I wonder what PageSpeed Insights uses, maybe we could align with that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. That did cross my mind, but I didn't implement it. I picked the 360x800 and 1920x1080 since they are the most common mobile and desktop dimensions currently. In 1f3b1c5 I've implemented the mobile and desktop aliases, and I've set their default values to correspond to what Lighthouse uses.

@westonruter
Copy link
Collaborator Author

Something else to consider, beyond adding support for deviceScaleFactor would be whether a mobile user agent should be sent when requesting with mobile viewport dimensions, and whether the same should be done for desktop. WP Rocket, for example, has different caches for mobile and desktop user agents, with device-specific optimizations applied. For non-responsive sites, it's common to send back entirely different HTML for desktop and mobile. So if we don't (eventually) also support full emulation then we won't get the expected results back.

@westonruter
Copy link
Collaborator Author

Thinking about this some more, I think we should also have a --emulate-device (-e) option which allows you to pass a predefined device name which would then populate the viewportWindow. This would supply the deviceScaleFactor and set the user agent and all that. We can still also provide a --window-viewport argument to override the specific dimensions, or if we don't want to emulate anything.

Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @westonruter, LGTM!

@felixarntz
Copy link
Collaborator

@westonruter Those ideas sound good to me, we could explore them in separate PRs. Though this could also wait until we find an actual use-case that would require us to support those.

@felixarntz felixarntz merged commit 544a8fc into main Nov 26, 2024
4 checks passed
@westonruter
Copy link
Collaborator Author

As it stands right now, specifying -w mobile won't necessarily result in what is expected since the pixel density won't be right and the user agent won't be sent as a mobile device, so I think it's important to support to get accurate results.

@westonruter
Copy link
Collaborator Author

Follow up: #166

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants