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(recorder): reattach toolbar if it was unmounted by framework hydration #32637

Merged
merged 6 commits into from
Sep 17, 2024

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Sep 16, 2024

Closes #32632. A side effect of Remix's hydration implementation is that it throws away the entire DOM. This is broadly discussed in remix-run/remix#4822 - there might be a fix in coming React versions, but who knows.
Besides breaking browser extensions, this also deletes our toolbar!
This PR fixes it by periodically checking in on x-pw-glass, and remounting it if it was unmounted. Hacky but effective!

@Skn0tt Skn0tt requested a review from dgozman September 16, 2024 14:05
@Skn0tt Skn0tt self-assigned this Sep 16, 2024
@@ -91,6 +92,11 @@ export class Highlight {

install() {
this._injectedScript.document.documentElement.appendChild(this._glassPaneElement);

this._glassPaneInterval = setInterval(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried using MutationObserver as the more canonical solution over setInterval, but couldn't get that to work.

Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

This seems to be only relevant to the recorder. Should we put the code there instead?

This comment has been minimized.

This comment has been minimized.

@Skn0tt
Copy link
Member Author

Skn0tt commented Sep 16, 2024

This seems to be only relevant to the recorder. Should we put the code there instead?

good call, done!

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test] › babel.spec.ts:135:5 › should not transform external

2 flaky ⚠️ [firefox-page] › page/page-event-request.spec.ts:169:3 › should return response body when Cross-Origin-Opener-Policy is set
⚠️ [webkit-library] › library/browsercontext-clearcookies.spec.ts:52:3 › should remove cookies by name

35465 passed, 659 skipped
✔️✔️✔️

Merge workflow run.

@Skn0tt Skn0tt merged commit b0f15b3 into microsoft:main Sep 17, 2024
29 of 30 checks passed
@akoenig
Copy link

akoenig commented Sep 17, 2024

@Skn0tt, thanks a lot for this change. How can I give it a test drive?

@Skn0tt
Copy link
Member Author

Skn0tt commented Sep 17, 2024

We publish a nightly release under the next label: https://www.npmjs.com/package/playwright/v/next
So try npm i playwright@next tomorrow to test drive.

@akoenig
Copy link

akoenig commented Sep 18, 2024

@Skn0tt, pretty sure I miss something, but I still receive:

CleanShot 2024-09-18 at 08 16 17@2x

I had @playwright/test in my dependencies. That is why i updated this package to 1.48.0-alpha-2024-09-18.

@Skn0tt
Copy link
Member Author

Skn0tt commented Sep 18, 2024

Right, you won't be able to get around that. It's just a consequence of how React's rehydration works. Does this cause any errors except for this warning though?

@akoenig
Copy link

akoenig commented Sep 18, 2024

@Skn0tt, the toolbar still disappears in the current alpha release. Unfortunately, the recent bugfix didn't resolve this issue (at least for me in my reproduction repository).

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.

[Bug]: Recorder Toolbar Hydration Problem
3 participants