fix: make the primary domain communicator a singleton per cypress instance #20653
Conversation
This file contains hidden or 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
Contributor
|
Thanks for taking the time to open a PR!
|
AtofStryker
changed the title
AtofStryker
force-pushed
the
fix_reify_error_issues
branch
from
5327199 to
b047a32
Compare
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||
…nts once in the primary and per spec bridge. Forward all message events to the respective communicator instance, if available.
AtofStryker
force-pushed
the
fix_reify_error_issues
branch
from
87d3d95 to
0f154b6
Compare
chrisbreiding
approved these changes
Mar 18, 2022
Comment on lines
32
to
33
| // currently used for tests, can be removed later | ||
| if (data?.actual) return |
Contributor
There was a problem hiding this comment.
This can be removed now.
Contributor
Author
There was a problem hiding this comment.
awesome. will push a commit that gets rid of this!
…is no longer needed
mschile
approved these changes
Mar 18, 2022
mjhenkes
approved these changes
Mar 18, 2022
mschile
pushed a commit
that referenced
this pull request
Mar 22, 2022
…tance (#20653) * fix: make the primary domain communicator a singleton per cypress instance * leverage a factory to make typescript happy with their super() constructor rules * remove singleton instance from communicator and only bind message events once in the primary and per spec bridge. Forward all message events to the respective communicator instance, if available. * remove data.actual reference in primary communicator onMessage as it is no longer needed
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.
User facing changelog
N/A
Additional details
When we reload tests, this errors tends to pop up anytime we edit tests that are dealing with errors in multi-domain. This is caused by multiple instances of the
primaryDomainCommunicatorbeing created, but rebinding the samewindow.top.addEventListener('message')on recreation. This leads to duplicatemessageevents. This was introduced in #20520 when theuserInvocationStackwas added to theprimaryDomainCommunicatorinstance here. The danglingprimaryDomainCommunicatorinstance would NOT have theuserInvocationStackset, which would fail to reify errors coming back from the other domain.The proposed fix here is to make theprimaryDomainCommunicatora singleton, so thewindow.top.addEventListener('message')is only bound once. Since the top reference should not change throughopenmode, this should work correctly.Update 3/17/22
Instead of creating a singleton, we will only bind the message handler once in the primary when global listeners are added, and once in the spec bridge on creation. This refactors the initialize methods to be onMessage handlers where message events get forwarded to whatever the current instance of the communicator exists
Currently not sure of a good way to test this yet. I was thinking of maybe adding an assertion to the rerun test, but that doesn't exactly work since the file is loaded back in.
the error before fix
after fix (rerunning consistently)
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?