feat: [Multi-domain]: Yield subject from `switchToDomain` #19936

mjhenkes · 2022-01-27T21:04:15Z

Closes [Multi-domain]: Handle Yielded subject from switchToDomain callback #19853

User facing changelog

n/a

Additional details

With this change we are yielding the last subject returned by the last run cy command or the return statement of synchronous execution. If the subject or return cannot be serialized we return undefined.

How has the user experience changed?

Users may now write tests like this

    it('yields a value', () => {
      cy.switchToDomain('foobar.com', () => {
        cy
        .get('[data-cy="dom-check"]')
        .invoke('text')
      }).should('equal', 'From a secondary domain')
    })

PR Tasks

Have tests been added/updated?
Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-01-27T21:04:17Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

mjhenkes · 2022-01-27T21:05:46Z

packages/driver/src/multi-domain/serializer.ts

+ * @param value any object to be serialized.
+ * @returns the serialized object as a string or undefined, if the object cannot be serialized.
+ */
+const serialize = (value: any): string => {


@AtofStryker, I know that you put together some level of serialization for the errors but this seemed different enough? Perhaps I should rename this subjectSerializer?

I just saw this comment after review. Yeah I think that is a good idea to distinguish between the two! I have a feeling we are going to have to address serialization pretty soon since it sounds like we are going to have quite a few and might need to see where we can consolidate on.

mjhenkes · 2022-01-27T21:06:56Z

packages/driver/src/multi-domain/serializer.ts

+ try {
+ serializedSubject = JSON.stringify(value, (key, value) => {
+ // If we encounter any unserializable values we want to abort the whole process.
+ if (isFunction(value) || isSymbol(value) || value === undefined) {


We can't port undefined using json, even trying to sub it out for our own custom string would require us to write our own parse function. I could transform undefined values into null but that may be confusing.

I wonder if this is more of a user experience type of thing. I wonder if assigning null on purpose would tell the end user that null was intentionally assigned because we couldn't serialize their yield. But that also might be confusing.

I think we probably need to discuss this with Ben and Brian. We talked about making unserialized values undefined, but not about objects that might have unserializable values within them, so I think we need to define that behavior. I'm not sure if throwing away the whole object is the right way to go or not.

mjhenkes · 2022-01-27T21:07:30Z

packages/driver/src/multi-domain/serializer.ts

+ * @returns the deserialized object
+ */
+const deserialize = (value: string|undefined): string|object|boolean|number|null|any[] => {
+ return value ? JSON.parse(value) : value


this code is pretty simple but i liked pairing a deserialize function with my serialize function.

mjhenkes · 2022-01-27T21:07:58Z

packages/driver/src/cypress/log.ts

@@ -50,7 +50,7 @@ const toSerializedJSON = function (attrs) {
 return value()
 }

- if (_.isFunction(value)) {
+ if (_.isFunction(value) || _.isSymbol(value)) {


This fixes an error where we're trying to log out an object containing a symbol

mjhenkes · 2022-01-27T21:10:28Z

packages/driver/src/cy/multi-domain/index.ts


- resolve()
+ resolve(deserializedSubject)


Here is where we resolve anything returned from the switchToDomain function. I'm not sure when it makes sense to synchronously return something from switchToDomain, but i'm matching the functionality of the then function blocks.

mjhenkes · 2022-01-27T21:11:02Z

packages/driver/src/cy/multi-domain/commands_manager.ts

- return command.deferred.resolve()
+ const deserializedSubject = deserialize(subject)
+
+ return command.deferred.resolve(deserializedSubject)


Here we're returning the last yielded subject from the last run cy command.

mjhenkes · 2022-01-27T21:12:17Z

packages/driver/src/multi-domain/commands.ts

@@ -14,7 +15,11 @@ export const handleCommands = (Cypress: Cypress.Cypress, cy: $Cy, specBridgeComm
 const id = command.get('id')
 const name = command.get('name')

- specBridgeCommunicator.toPrimary('command:end', { id, name })
+ let serializedSubject = serialize(cy.state('subject'))


It might be possible to avoid doing this if we aren't the last command in the queue. I'll investigate that.

I could do this, is this crazy? We're getting the last command in the queue and comparing ids

Suggested change

let serializedSubject = serialize(cy.state('subject'))

let serializedSubject: string | undefined = undefined

if (cy.queue.at(cy.queue.length - 1).get('id') === id) {

serializedSubject = serialize(cy.state('subject'))

}

Do we ever need to yield something back that isn't the last command? I don't think it's crazy and it could save on serialization costs if other commands are yielding subjects.

Seems like a good idea to me. Let's go ahead and add a cy.queue.last() method or even a cy.queue.isLast(id) method.

AtofStryker

Just a few small comments and suggestions. Looking good!

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts

AtofStryker · 2022-01-28T16:31:20Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts

+ })
+ }).should('equal', undefined)
+ })
+


I wonder if we want to do something similar with Errors here since they aren't serializable through the structured clone algorithm in Firefox. Since we already attempt to serialize errors to objects and back to errors in the primary, I wonder if we want to leverage the util function here in the serializer.

Or we can also just return undefined if an error is yielded to be consistent with the rest of the unserializable attributes. I'm also not sure why someone would want to yield an error 🤷‍♂️ .

it('yields undefined if an object contains an error', () => { cy.switchToDomain('foobar.com', () => { cy.wrap({ key: new Error('Boom goes the dynamite'), }) }).should('equal', undefined) })

I would expect an unserializable values to be undefined, not the whole object itself. So I would think the 3 above tests would yield an empty object ({}) instead of undefined. What happens if there are other serializable values in the object like { key: undefined, otherKey: 'defined' }?

AtofStryker · 2022-01-28T16:38:27Z

packages/driver/src/multi-domain/serializer.ts

+ * @param value a string
+ * @returns the deserialized object
+ */
+const deserialize = (value: string|undefined): string|object|boolean|number|null|any[] => {


Suggested change

const deserialize = (value: string|undefined): string|object|boolean|number|null|any[] => {

const deserializeSubject = (value: string|undefined): string|object|boolean|number|null|any[] => {

AtofStryker · 2022-01-28T16:38:37Z

packages/driver/src/multi-domain/serializer.ts

+ * @param value any object to be serialized.
+ * @returns the serialized object as a string or undefined, if the object cannot be serialized.
+ */
+const serialize = (value: any): string | undefined => {


Since we now have multiple serializers going on, should we prefix this to make the functions intent clearer?

Suggested change

const serialize = (value: any): string | undefined => {

const serializeSubject = (value: any): string | undefined => {

AtofStryker · 2022-01-28T16:43:07Z

packages/driver/src/multi-domain/serializer.ts

+ try {
+ serializedSubject = JSON.stringify(value, (key, value) => {
+ // If we encounter any unserializable values we want to abort the whole process.
+ if (isFunction(value) || isSymbol(value) || value === undefined) {


Total nitpick, but more of a readability thing here. I think this also allows for void operator support, but I wouldn't think that would be applicable here.

Suggested change

if (isFunction(value) || isSymbol(value) || value === undefined) {

if (isFunction(value) || isSymbol(value) || isUndefined(value)) {

We also might need to add isError here as well.

AtofStryker · 2022-01-28T16:48:22Z

packages/driver/src/multi-domain/serializer.ts

+ try {
+ serializedSubject = JSON.stringify(value, (key, value) => {
+ // If we encounter any unserializable values we want to abort the whole process.
+ if (isFunction(value) || isSymbol(value) || value === undefined) {


I wonder if this is more of a user experience type of thing. I wonder if assigning null on purpose would tell the end user that null was intentionally assigned because we couldn't serialize their yield. But that also might be confusing.

packages/driver/src/multi-domain/commands.ts

AtofStryker · 2022-01-28T16:56:43Z

packages/driver/src/multi-domain/domain_fn.ts


- specBridgeCommunicator.toPrimary('ran:domain:fn')
+ specBridgeCommunicator.toPrimary('ran:domain:fn', { subject: serialize(subject) })


does leveraging the serializer argument in toPrimary work here?

Suggested change

specBridgeCommunicator.toPrimary('ran:domain:fn', { subject: serialize(subject) })

specBridgeCommunicator.toPrimary('ran:domain:fn', { subject }, serialize)

No, but i should make it work, i just realized I'm doing double serialization

AtofStryker · 2022-01-28T17:01:01Z

packages/driver/src/multi-domain/commands.ts

@@ -14,7 +15,11 @@ export const handleCommands = (Cypress: Cypress.Cypress, cy: $Cy, specBridgeComm
 const id = command.get('id')
 const name = command.get('name')

- specBridgeCommunicator.toPrimary('command:end', { id, name })
+ let serializedSubject = serialize(cy.state('subject'))


Do we ever need to yield something back that isn't the last command? I don't think it's crazy and it could save on serialization costs if other commands are yielding subjects.

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts

Co-authored-by: Bill Glesias <bglesias@gmail.com>

chrisbreiding · 2022-01-28T20:42:34Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts

+ it('yields the cy value even if a return is present', () => {
+ cy.switchToDomain('foobar.com', () => {
+ cy
+ .get('[data-cy="dom-check"]')
+ .invoke('text')
+
+ return 'text'
+ }).should('equal', 'From a secondary domain')
+ })


This might require some discussion, but I would expect this to behave similar to .then, where we throw an error in this case, since it's probably not what the user expects the yielded value to be.

cy .visit('/test.html') .then(() => { cy.get('h1').invoke('text') return 'text' }) .should('equal', 'text')

results in this error:

That makes sense, I'll look into it

chrisbreiding · 2022-01-28T20:46:51Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_spec.ts

+ })
+ }).should('equal', undefined)
+ })
+


I would expect an unserializable values to be undefined, not the whole object itself. So I would think the 3 above tests would yield an empty object ({}) instead of undefined. What happens if there are other serializable values in the object like { key: undefined, otherKey: 'defined' }?

chrisbreiding · 2022-01-28T20:58:20Z

packages/driver/src/multi-domain/commands.ts

@@ -14,7 +15,11 @@ export const handleCommands = (Cypress: Cypress.Cypress, cy: $Cy, specBridgeComm
 const id = command.get('id')
 const name = command.get('name')

- specBridgeCommunicator.toPrimary('command:end', { id, name })
+ let serializedSubject = serialize(cy.state('subject'))


Seems like a good idea to me. Let's go ahead and add a cy.queue.last() method or even a cy.queue.isLast(id) method.

chrisbreiding · 2022-01-28T21:08:16Z

packages/driver/src/multi-domain/serializer.ts

+ try {
+ serializedSubject = JSON.stringify(value, (key, value) => {
+ // If we encounter any unserializable values we want to abort the whole process.
+ if (isFunction(value) || isSymbol(value) || value === undefined) {


I think we probably need to discuss this with Ben and Brian. We talked about making unserialized values undefined, but not about objects that might have unserializable values within them, so I think we need to define that behavior. I'm not sure if throwing away the whole object is the right way to go or not.

packages/driver/src/cypress/error_messages.ts

packages/driver/src/util/queue.ts

Co-authored-by: Matt Schile <mschile@gmail.com>

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts

AtofStryker · 2022-02-02T18:45:28Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts

+
+ cy.switchToDomain('foobar.com', () => {
+ cy.wrap({
+ key: new Error('Boom goes the dynamite'),


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.

packages/driver/src/cy/multi-domain/failedSerializeSubjectProxy.ts

AtofStryker · 2022-02-02T18:51:24Z

packages/driver/src/cy/multi-domain/failedSerializeSubjectProxy.ts

+ */
+ get (target, prop, receiver) {
+ // These properties are required to avoid failing prior to attempting to use the subject.
+ if (


Could we refactor this to a list on constants like const unserializableSubjects = ['then', 'jquery', ...] and do something like unserializableSubjects.includes(prop) since we are working with strings? It might make this a bit easier to read and maintain in the future.

AtofStryker · 2022-02-02T18:57:05Z

packages/driver/src/cy/multi-domain/index.ts

+ // This handles when a subject is returned synchronously
+ if (failedToSerializeSubjectOfType) {
+ return resolve(
+ failedToSerializeSubject(failedToSerializeSubjectOfType),
+ )
+ }


Total nit/preference here. I'm going to apologize for the formatting in this suggestion in advance 😅

Suggested change

// This handles when a subject is returned synchronously

if (failedToSerializeSubjectOfType) {

return resolve(

failedToSerializeSubject(failedToSerializeSubjectOfType),

)

}

// This handles when a subject is returned synchronously

const serializedSubject = failedToSerializeSubjectOfType ?

failedToSerializeSubject(failedToSerializeSubjectOfType) :

subject

resolve(serializedSubject)

I updated with a single line variant fo this suggestion

AtofStryker · 2022-02-02T19:02:15Z

packages/driver/src/multi-domain/communicator.ts

 const { isDom } = $dom

 if (_.isError(value)) {
- const serializedError = _.mapValues(clone(value), serializeForPostMessage)
+ const serializedError = _.mapValues(clone(value), preprocessErrorForPostMessage)


Do we still want to call this a serializedError? I was almost thinking processedError but that almost sounds more confusing 😅

Suggested change

const serializedError = _.mapValues(clone(value), preprocessErrorForPostMessage)

const processedError = _.mapValues(clone(value), preprocessErrorForPostMessage)

Also, if we are using this for only errors now and not generic serialization, I wonder if we can gut like 90% of this function and just imply that value is of type Error or if its undefined just do nothing?

I went with serializableError. I do wonder what we could trim out from this function, but haven't dug into what could be in error that needs to be serialized since we do call this function recursively to serialize anything in error.

packages/driver/src/multi-domain/domain_fn.ts

AtofStryker · 2022-02-02T19:14:44Z

packages/driver/src/cypress/error_messages.ts

@@ -1710,6 +1710,32 @@ export default {

 This is likely because the data argument specified is not serializable. Note that functions and DOM objects cannot be serialized.`,
 },
+ callback_mixes_sync_and_async: {


I wonder if something like callback_mixes_sync_and_commands is more appropriate? Not entirely sure 🤔

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts

Co-authored-by: Bill Glesias <bglesias@gmail.com>

mschile · 2022-02-03T15:22:05Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts

+ cy
+ .get('[data-cy="dom-check"]')
+ }).invoke('text')
+ .should('equal', 'From a secondary domain')


You can probably remove these shoulds now since they aren't getting hit.

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.

packages/driver/src/cy/multi-domain/failedSerializeSubjectProxy.ts

…y.ts Co-authored-by: Matt Schile <mschile@gmail.com>

AtofStryker

🚀

AtofStryker · 2022-02-03T16:34:00Z

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts

+ cy
+ .get('[data-cy="dom-check"]')
+ }).invoke('text')
+ .should('equal', 'From a secondary domain')


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.

mjhenkes added 4 commits January 27, 2022 13:41

first pass at yielding a subject

6e68473

Merge branch 'feature-multidomain' into yield-subject

0c3ddcb

Remove logs and add comments

4ce0ccc

remove duplicate test

a091a21

mjhenkes requested a review from a team as a code owner January 27, 2022 21:04

mjhenkes requested review from jennifer-shehane and removed request for a team January 27, 2022 21:04

mjhenkes requested review from chrisbreiding, AtofStryker and mschile and removed request for jennifer-shehane January 27, 2022 21:04

mjhenkes commented Jan 27, 2022

View reviewed changes

mjhenkes added the topic: cy.origin Problems or enhancements related to cy.origin command label Jan 27, 2022

mjhenkes changed the title ~~[Multi-domain]: Yield subject from switchToDomain~~ feat: [Multi-domain]: Yield subject from switchToDomain Jan 27, 2022

mjhenkes added 2 commits January 27, 2022 15:43

Add debug logging for aborted

8e14be9

Merge branch 'feature-multidomain' into yield-subject

34222f5

AtofStryker reviewed Jan 28, 2022

View reviewed changes

Apply suggestions from code review

c5ca5c2

Co-authored-by: Bill Glesias <bglesias@gmail.com>

chrisbreiding requested changes Jan 28, 2022

View reviewed changes

mjhenkes added 4 commits January 31, 2022 11:55

New strategy for serializing subjects

40f2ad6

handle 'ran:domain:fn' event

b2981e5

Fix lint

bf6522c

Now with proxy error handling!

24bbf59

add a param for the malformed should

66f1de4

mschile reviewed Feb 2, 2022

View reviewed changes

packages/driver/src/cypress/error_messages.ts Outdated Show resolved Hide resolved

mschile reviewed Feb 2, 2022

View reviewed changes

packages/driver/src/cypress/error_messages.ts Outdated Show resolved Hide resolved

mschile reviewed Feb 2, 2022

View reviewed changes

packages/driver/src/cypress/error_messages.ts Outdated Show resolved Hide resolved

mschile reviewed Feb 2, 2022

View reviewed changes

packages/driver/src/cypress/error_messages.ts Outdated Show resolved Hide resolved

mschile reviewed Feb 2, 2022

View reviewed changes

packages/driver/src/util/queue.ts Outdated Show resolved Hide resolved

mjhenkes and others added 2 commits February 2, 2022 11:53

Apply suggestions from code review

905eb5b

Co-authored-by: Matt Schile <mschile@gmail.com>

update test to work cross browsers

2fe2f7f

AtofStryker self-requested a review February 2, 2022 18:04

mjhenkes and others added 3 commits February 2, 2022 12:09

fix test strings

9ff8fde

Update packages/driver/src/util/queue.ts

c834c9b

Co-authored-by: Matt Schile <mschile@gmail.com>

optional chaining !

ee14266

AtofStryker reviewed Feb 2, 2022

View reviewed changes

mjhenkes commented Feb 2, 2022

View reviewed changes

packages/driver/cypress/integration/e2e/multi-domain/multi_domain_yield_spec.ts Outdated Show resolved Hide resolved

mjhenkes and others added 4 commits February 2, 2022 14:13

Apply suggestions from code review

944f08d

Co-authored-by: Bill Glesias <bglesias@gmail.com>

code review changes

8ebc545

Whoops

0a041f6

whoops, renamed the wrong test file

78fcb01

chrisbreiding self-requested a review February 3, 2022 15:15

AtofStryker self-requested a review February 3, 2022 15:20

mschile reviewed Feb 3, 2022

View reviewed changes

chrisbreiding approved these changes Feb 3, 2022

View reviewed changes

mschile reviewed Feb 3, 2022

View reviewed changes

packages/driver/src/cy/multi-domain/failedSerializeSubjectProxy.ts Outdated Show resolved Hide resolved

Update packages/driver/src/cy/multi-domain/failedSerializeSubjectProx…

9f17a9a

…y.ts Co-authored-by: Matt Schile <mschile@gmail.com>

AtofStryker approved these changes Feb 3, 2022

View reviewed changes

mschile approved these changes Feb 3, 2022

View reviewed changes

mjhenkes merged commit c58a0ee into feature-multidomain Feb 3, 2022

mjhenkes deleted the yield-subject branch February 3, 2022 17:56

cypress-bot bot mentioned this pull request Feb 3, 2022

[Multi-domain]: Handle Yielded subject from switchToDomain callback #19853

Closed

AtofStryker mentioned this pull request Feb 7, 2022

fix: secondary domain error serialization in firefox #20080

Merged

5 tasks

	const deserialize = (value: string\|undefined): string\|object\|boolean\|number\|null\|any[] => {
	const deserializeSubject = (value: string\|undefined): string\|object\|boolean\|number\|null\|any[] => {

	const serialize = (value: any): string \| undefined => {
	const serializeSubject = (value: any): string \| undefined => {

	if (isFunction(value) \|\| isSymbol(value) \|\| value === undefined) {
	if (isFunction(value) \|\| isSymbol(value) \|\| isUndefined(value)) {


		specBridgeCommunicator.toPrimary('ran:domain:fn')
		specBridgeCommunicator.toPrimary('ran:domain:fn', { subject: serialize(subject) })

	const serializedError = _.mapValues(clone(value), preprocessErrorForPostMessage)
	const processedError = _.mapValues(clone(value), preprocessErrorForPostMessage)

feat: [Multi-domain]: Yield subject from switchToDomain #19936

feat: [Multi-domain]: Yield subject from switchToDomain #19936

Conversation

mjhenkes commented Jan 27, 2022

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: [Multi-domain]: Yield subject from `switchToDomain` #19936

feat: [Multi-domain]: Yield subject from `switchToDomain` #19936