-
Notifications
You must be signed in to change notification settings - Fork 127
enhancement: implement getContext method for device emulation #27
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
base: main
Are you sure you want to change the base?
Changes from all commits
09f999a
b64214e
4ffaa46
fb498f8
0f6ec53
e066278
30f4cb2
436f362
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,21 @@ | ||
| import { chromium, Browser, LaunchOptions } from "playwright"; | ||
| import { | ||
| chromium, | ||
| Browser, | ||
| LaunchOptions, | ||
| devices, | ||
| BrowserContext, | ||
| } from "playwright"; | ||
| import BrowserProvider from "@/types/browser-providers/types"; | ||
|
|
||
| export class LocalBrowserProvider extends BrowserProvider<Browser> { | ||
| options: Omit<Omit<LaunchOptions, "headless">, "channel"> | undefined; | ||
| session: Browser | undefined; | ||
|
|
||
| constructor(options?: Omit<Omit<LaunchOptions, "headless">, "channel">) { | ||
| super(); | ||
| this.options = options; | ||
| } | ||
|
|
||
| async start(): Promise<Browser> { | ||
| const launchArgs = this.options?.args ?? []; | ||
| const browser = await chromium.launch({ | ||
|
|
@@ -19,13 +27,34 @@ export class LocalBrowserProvider extends BrowserProvider<Browser> { | |
| this.session = browser; | ||
| return this.session; | ||
| } | ||
|
|
||
| async close(): Promise<void> { | ||
| return await this.session?.close(); | ||
| } | ||
|
|
||
| public getSession() { | ||
| if (!this.session) { | ||
| return null; | ||
| } | ||
| return this.session; | ||
| } | ||
|
|
||
| public async getContext( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add this method on the abstract base class that they inherit from ? |
||
| device: string = "desktop" | ||
| ): Promise<BrowserContext | null> { | ||
| if (!this.session) return null; | ||
|
|
||
| if (device === "mobile") { | ||
| const iPhone = devices["iPhone 12"]; | ||
| return await this.session.newContext({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pass in the entire device object, don't specify individual params here this.session.newContext({...devices[device]})
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried that - that wouldn't work because, there's this property where When we pass the entire object, it will throw an error like:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason behind that is
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And |
||
| userAgent: iPhone.userAgent, | ||
| viewport: { | ||
| width: iPhone.viewport.width + 50, | ||
| height: iPhone.viewport.height + 50, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| return await this.session.newContext(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| import { Browser } from "playwright"; | ||
| import { Browser, BrowserContext } from "playwright"; | ||
|
|
||
| abstract class BrowserProvider<T> { | ||
| abstract session: unknown; | ||
| abstract start(): Promise<Browser>; | ||
| abstract close(): Promise<void>; | ||
| abstract getSession(): T|null; | ||
| abstract getSession(): T | null; | ||
| abstract getContext(device?: string): Promise<BrowserContext | null>; | ||
| } | ||
|
|
||
| export default BrowserProvider; |
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.
Can you take the types directly from the devices offered from the devices list ?
Something like
private clientType?: keyof typeof devices;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.
In the constructor too can you assign the
clientTypeon the object instance ?Like
this.clientType = params.clientTypeThere 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 - I tried this, but it wouldn't give actual suggestions to user - reason being, playwright doesnt do
exporton Devices type. So, considering user perspective - asking user to select mobile / desktop - would make more sense since we're not able to provide suggestions.