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.
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
feat: [Multi-domain]: Yield subject from
switchToDomain
#19936feat: [Multi-domain]: Yield subject from
switchToDomain
#19936Changes from 20 commits
6e68473
0c3ddcb
4ce0ccc
a091a21
8e14be9
34222f5
c5ca5c2
40f2ad6
b2981e5
bf6522c
24bbf59
ffe38e7
6a42881
19fb21c
66f1de4
905eb5b
2fe2f7f
9ff8fde
c834c9b
ee14266
944f08d
8ebc545
0a041f6
78fcb01
9f17a9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably remove these
should
s now since they aren't getting hit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like having them there to show how a user might think this test works, but doesn't. Almost like a documentation artifact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope here, but I am also wondering if we should do something similar with test errors. In other words, only try to preprocess the error if the browser can't serialize it through
postMessage
. Otherwise, just send it as is as opposed to always preprocessing the error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic, as minimal as it is, is duplicated in index.ts (see below). Is it worth trying to do some special handling of the end:command and ran:domain:fn events in the communicator to swap in the proxy for the subject before these commands fire? (I think that could work?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we refactor this to a list on constants like
const unserializableSubjects = ['then', 'jquery', ...]
and do something likeunserializableSubjects.includes(prop)
since we are working with strings? It might make this a bit easier to read and maintain in the future.