-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
feat: add page.emulateNetworkConditions #6759
Conversation
@sadym-chromium PTAL as well |
docs/api.md
Outdated
- `download` <[number]> Download speed (bytes/s), `-1` to disable | ||
- `upload` <[number]> Upload speed (bytes/s), `-1` to disable | ||
- `latency` <[number]> Latency (ms), `0` to disable | ||
- `connectionType` <?["none"| "cellular2g"| "cellular3g"| "cellular4g"| "bluetooth"| "ethernet"| "wifi"| "wimax"| "other"] Optional connection type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a bit more clarification for the params here? Are params download
, upload
and latency
optional in case of having connectionType
? Or why the connectionType
needed at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to the predefined network conditions in NetworkConditions.ts but only changes what navigator.connection.type
returns. But this seems to be unsupported on desktop anyways so maybe it's best to just omit it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omitting seems reasonable for me.
Another option is to rename it to something like connectionTypeDescription
and to explicitly describe what it intended to do.
UPD: having networkConditionsMap: NetworkConditionsMap
with quite close naming and very different meaning, I'd vote for omitting to prevent misunderstanding of the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about the new naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thanks
40e05e7
to
d3ec8a1
Compare
@@ -1441,6 +1463,29 @@ await page.evaluate(() => matchMedia('print').matches); | |||
// → false | |||
``` | |||
|
|||
#### page.emulateNetworkConditions(networkConditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jschfflr Thanks for working on this feature. As someone looking to switch to using this API to emulate offline testing, I wasn't sure how to transition from Network.emulateNetworkConditions
(which accepted an offline
property) to this API. Is calling page.setOfflineMode(true)
all that's needed?
Unfortunately setOfflineMode
docs also don't go into detail.
It might be worth mentioning how this should work. If you can clarify I'm happy to update the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a look at this - I'll get back to you in a sec :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, page.setOfflineMode(true)
is all you need. I'll update the documentation accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if that helps or if I should further clarify their relationship: #7446
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll close this comment to move the discussion to the pull request.
@mathiasbynens Please take a look!