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

fix(electron): browser window ids out of sync #8759

Merged

Conversation

samuelmaddock
Copy link
Contributor

@samuelmaddock samuelmaddock commented Sep 7, 2021

fixes #6760
fixes #6748

Playwright's Electron implementation is keeping track of BrowserWindow IDs itself which has issues that cause it to go out of sync. This has been a blocker for complex Electron apps using multiple windows or BrowserViews.

I've introduced a new API to Electron which will allow using the DevTools Target ID as a stable lookup identifier for WebContents: electron/electron#29399


This PR is set as a draft to receive feedback and wait for Electron changes to be merged and released:

v12.2.0 has been released now with these changes. This PR upgrade to it since it's the minimum supported version.

@ghost
Copy link

ghost commented Sep 7, 2021

CLA assistant check
All CLA requirements met.

@dgozman dgozman added the CQ1 label Sep 9, 2021
@dgozman dgozman marked this pull request as ready for review September 9, 2021 20:01
@dgozman
Copy link
Contributor

dgozman commented Sep 9, 2021

@samuelmaddock Could you please rebase this (there are merge conflicts) so I can trigger the bots? I've also undrafted for the bots to pick this up.

@mxschmitt mxschmitt added CQ1 and removed CQ1 labels Sep 10, 2021
@samuelmaddock samuelmaddock force-pushed the fix/electron-browserwindow branch 2 times, most recently from f83851a to 23ad986 Compare October 1, 2021 00:01
@samuelmaddock
Copy link
Contributor Author

Changes have been rebased and now targets Electron v12.2.0 which is the minimum version including the new webContents.fromDevToolsTargetId(targetId) API.

@samuelmaddock
Copy link
Contributor Author

TS error after upgrading Electron:

> playwright-internal@1.16.0-next tsc
> tsc -p .

node_modules/electron/electron.d.ts:6653:21 - error TS2694: Namespace 'NodeJS' has no exported member 'Require'.

6653     require: NodeJS.Require;
                         ~~~~~~~


Found 1 error.

It seems like @types/node needs to be updated. After bumping it to ^12.12.6, I receive 57 errors. What's the preferred path forward in this case? New PR to update @types/node?

@mxschmitt
Copy link
Member

@samuelmaddock we can split it in two. I'll take care that #8837 lands soon which does exactly that, then your PR should pass.

@pavelfeldman
Copy link
Member

I landed the roll, you can rebase this PR and we will merge.

@mxschmitt
Copy link
Member

For the record: After this patch the minimum required versions of Electron are:

  • v12.2.0+
  • v13.4.0+
  • v14.0.0+
  • v15.0.0+

@pavelfeldman pavelfeldman merged commit 1b83f3e into microsoft:master Oct 4, 2021
@samuelmaddock samuelmaddock deleted the fix/electron-browserwindow branch October 6, 2021 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants