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: close WebDriver BiDi session on disconnect #11470

Merged
merged 6 commits into from Nov 30, 2023
Merged

Conversation

sadym-chromium
Copy link
Collaborator

@sadym-chromium sadym-chromium commented Nov 30, 2023

  1. browser.disconnect returns Promise<void> instead of void. This is required to be able to properly close BiDi session before disconnecting.
  2. BiDi browser.disconnect sends session.end before disconnecting.
  3. BiDi over CDP closes CDP connection when closed.
  4. Fix BiDi browser close: dispose connection after the browser is closed.

Preparation for supporting puppeteer.connect by Firefox.

@sadym-chromium sadym-chromium requested review from Lightning00Blade and OrKoN and removed request for Lightning00Blade November 30, 2023 10:10
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

Have not you said it's a non-breaking change?

@sadym-chromium
Copy link
Collaborator Author

sadym-chromium commented Nov 30, 2023

Have not you said it's a non-breaking change?

Technically it is not breaking, as behavior has not changed and it should not change any existing workflows, but the API has changed.

@sadym-chromium
Copy link
Collaborator Author

Have not you said it's a non-breaking change?

I can switch description to non-breaking, WDYT?

@sadym-chromium
Copy link
Collaborator Author

Converting to draft, as some tests are failing

@OrKoN
Copy link
Collaborator

OrKoN commented Nov 30, 2023

There are two concerns: 1) every breaking change results in a major puppeteer version which we don't want to bump so often 2) at the same time it does not look like it deserves a major version since there is no breaking behavior.

@OrKoN OrKoN changed the title feat!: close BiDi session properly fix: close WebDriver BiDi session on disonnect Nov 30, 2023
@OrKoN OrKoN changed the title fix: close WebDriver BiDi session on disonnect fix: close WebDriver BiDi session on disconnect Nov 30, 2023
@OrKoN
Copy link
Collaborator

OrKoN commented Nov 30, 2023

Let's go with fix: close WebDriver BiDi session on disconnect. I think this change should not break anything for existing users.

@sadym-chromium sadym-chromium marked this pull request as ready for review November 30, 2023 14:18
Maksim Sadym added 6 commits November 30, 2023 15:18
`browser.disconnect` returns `Promise<void>` instead of `void`. This is required ti be able to properly close BiDi session before disconnecting.
This reverts commit a7b3995db4c3f7d973be024eb6799d276944008f.
@sadym-chromium sadym-chromium enabled auto-merge (squash) November 30, 2023 14:18
@sadym-chromium sadym-chromium merged commit a66d029 into main Nov 30, 2023
52 checks passed
@sadym-chromium sadym-chromium deleted the sadym/disconnect branch November 30, 2023 14:27
@release-please release-please bot mentioned this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants