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 13 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.
This seems incomplete since it doesn't have an expected parameter. As a side note, when I see these
should
s I think it is going to be a valid assertion.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.
it is a malformed should, i'll add another parameter, but the mere act of calling the .should will cause the proxy to throw an 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.
When I see the should I'm assuming that it's a valid assertion and something you are expecting to pass. I feels odd to me to be using the
should
to invoke the subject to cause an 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.
I've update the test to look more realistic, but regardless simply hanging a .should off a switchToDomain call when the subject failed to serialize will cause an error to throw.
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.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.
These special props were found by trial and error. it is possible that more will have to be added as commands evolve.
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.
Total nit/preference here. I'm going to apologize for the formatting in this suggestion in advance 😅
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 updated with a single line variant fo this suggestion