Simplify setup in happoPlaywright.init #9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This function is deprecated and unused. We can delete the code here to reduce complexity.
While debugging things for a customer, we found that `beforeAll` doesn't give us a context that we can use. While running the local tests, I found that I could initialize the takeDOMSnapshot function lazily when needed. This will simplify setup for users of this library. I'm making this a "soft" breaking change by generating a warning log if you pass a context to happoPlaywright.init. This way people can discover the simplification without much disruption.
trotzig
force-pushed
the
before-all
branch
4 times, most recently
from
ded9d7b to
f9141d0
Compare
trotzig
temporarily deployed
to
Happo secrets
GitHub Actions
Inactive
trotzig
temporarily deployed
to
Happo secrets
GitHub Actions
Inactive
So that we have some test coverage in this repo.
lencioni
reviewed
Dec 18, 2024
| await contextOrPage.addInitScript({ path: pathToBrowserBuild }); | ||
| await contextOrPage.addInitScript(` | ||
| window.addEventListener('load', () => { | ||
| window.happoTakeDOMSnapshot.init(window); |
There was a problem hiding this comment.
I was wondering if we still needed to call this init function somewhere, but I found the function definition and discovered that it is now a noop, so removing this should be fine https://github.com/happo/happo-e2e/blob/5903e00c9dd2bb4d5a9cf9c2994181668ddfc634/takeDOMSnapshot.js#L347-L351
However, if it is possible for someone to have an updated version of happo-playwright but an old version of happo-e2e, they may run into a confusing situation. I'm not entirely sure what that experience would be like, but if we think it is likely that someone will run into this, then maybe we should make an adjustment to help them out.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By lazy-loading the
takeDOMSnapshotfunction, we can remove the need to pass in a page or context inbeforeAll.