fix: preserve cache by using credentialless proxy #72
Conversation
| return [ | ||
| undefined, | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| async () => {}, |
There was a problem hiding this comment.
The empty functions are still returned in order to prevent repeating ifs like
if (close) {
await close();
}
| page.once('close', async () => { | ||
| this.activePages--; | ||
|
|
||
| await close(); |
There was a problem hiding this comment.
This is quite unsafe because the page may open popups and get closed after opening them.
There was a problem hiding this comment.
plus this can crash the entire process if close() throws for some unforeseeable reason
There was a problem hiding this comment.
plus this can crash the entire process if
close()throws for some unforeseeable reason
Yes, that's what we want. If it failed to close the server (which should never happen) then it leaks resources which means we want to crash.
There was a problem hiding this comment.
This is quite unsafe because the page may open popups and get closed after opening them.
Yeah, I see. But at the same time, closing those extra pages as well might be unexpected for users. Let's live with it and see if it causes any problems. The browsers have a hard kill interval anyway.
| } catch (error: any) { | ||
| log.exception(error, 'Failed to close context.'); | ||
| } finally { | ||
| await close(); |
| @@ -193,9 +193,6 @@ describe('Plugins', () => { | |||
| }); | |||
|
|
|||
| browser = await plugin.launch(context); | |||
| const argWithProxy = context.launchOptions?.args?.find((arg) => arg.includes('--proxy-server=')); | |||
There was a problem hiding this comment.
These expectations have been removed because we check the end IP anyway. It should not check whether it goes through a local proxy server or not. Cache checks should be separate ones.
|
looks like the tests got stuck |
|
@B4nan Jest fails to properly detect open handles (this is one of the reasons I believe we shouldn't use Jest), it hangs indefinitely and reports unreffed (!) servers. I've removed the open handles check, should exit now. |
|
Firefox crashes for some reason, dunno why. Good to merge otherwise. |
| // Launches unused browser just to get the browser version. | ||
| const inactiveBrowser = await this.library.launch(launchOptions); | ||
| this._browserVersion = inactiveBrowser.version(); | ||
| inactiveBrowser.close().catch((error) => { |
There was a problem hiding this comment.
not awaiting here is on purpose? if so, what is the reason?
There was a problem hiding this comment.
I'm +1 for awaiting here but Ondra had said that we should skip this so I followed.
There was a problem hiding this comment.
We don't want to slow down just to make the version work. Nothing more really.
There was a problem hiding this comment.
Alternatively we could use CDP for this https://chromedevtools.github.io/devtools-protocol/tot/Browser/#method-getVersion but IIRC we cannot connect to CDP with persistent context, that's a playwright limitation.
| page.once('close', async () => { | ||
| this.activePages--; | ||
|
|
||
| await close(); |
There was a problem hiding this comment.
This is quite unsafe because the page may open popups and get closed after opening them.
Yeah, I see. But at the same time, closing those extra pages as well might be unexpected for users. Let's live with it and see if it causes any problems. The browsers have a hard kill interval anyway.
|
@mnmkng I have to head out to uni, feel free to merge this or I'll merge this tomorrow afternoon |
I need to test this firstdone