Skip to content
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 #19936

Merged
merged 25 commits into from
Feb 3, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6e68473
first pass at yielding a subject
mjhenkes Jan 27, 2022
0c3ddcb
Merge branch 'feature-multidomain' into yield-subject
mjhenkes Jan 27, 2022
4ce0ccc
Remove logs and add comments
mjhenkes Jan 27, 2022
a091a21
remove duplicate test
mjhenkes Jan 27, 2022
8e14be9
Add debug logging for aborted
mjhenkes Jan 27, 2022
34222f5
Merge branch 'feature-multidomain' into yield-subject
mjhenkes Jan 27, 2022
c5ca5c2
Apply suggestions from code review
mjhenkes Jan 28, 2022
40f2ad6
New strategy for serializing subjects
mjhenkes Jan 31, 2022
b2981e5
handle 'ran:domain:fn' event
mjhenkes Jan 31, 2022
bf6522c
Fix lint
mjhenkes Jan 31, 2022
24bbf59
Now with proxy error handling!
mjhenkes Feb 2, 2022
ffe38e7
Break yields tests out into their own file.
mjhenkes Feb 2, 2022
6a42881
updated test
mjhenkes Feb 2, 2022
19fb21c
Update packages/driver/src/cy/multi-domain/failedSerializeSubjectProx…
mjhenkes Feb 2, 2022
66f1de4
add a param for the malformed should
mjhenkes Feb 2, 2022
905eb5b
Apply suggestions from code review
mjhenkes Feb 2, 2022
2fe2f7f
update test to work cross browsers
mjhenkes Feb 2, 2022
9ff8fde
fix test strings
mjhenkes Feb 2, 2022
c834c9b
Update packages/driver/src/util/queue.ts
mjhenkes Feb 2, 2022
ee14266
optional chaining !
mjhenkes Feb 2, 2022
944f08d
Apply suggestions from code review
mjhenkes Feb 2, 2022
8ebc545
code review changes
mjhenkes Feb 2, 2022
0a041f6
Whoops
mjhenkes Feb 2, 2022
78fcb01
whoops, renamed the wrong test file
mjhenkes Feb 3, 2022
9f17a9a
Update packages/driver/src/cy/multi-domain/failedSerializeSubjectProx…
mjhenkes Feb 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -278,4 +278,93 @@ describe('multi-domain', { experimentalSessionSupport: true, experimentalMultiDo

it('short circuits the secondary domain command queue when "done()" is called early')
})

describe('yields', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I break these out to their own file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

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')
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screen Shot 2022-01-28 at 3 41 34 PM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I'll look into it


it('yields synchronously', () => {
cy.switchToDomain('foobar.com', () => {
return 'From a secondary domain'
}).should('equal', 'From a secondary domain')
})

AtofStryker marked this conversation as resolved.
Show resolved Hide resolved
it('yields undefined if subject cannot be serialized', () => {
cy.switchToDomain('foobar.com', () => {
cy
.get('[data-cy="dom-check"]')
}).should('equal', undefined)
})

it('yields undefined if an object contains undefined', () => {
cy.switchToDomain('foobar.com', () => {
cy.wrap({
key: undefined,
})
}).should('equal', undefined)
})

it('yields undefined if an object contains a function', () => {
cy.switchToDomain('foobar.com', () => {
cy.wrap({
key: () => {
return 'whoops'
},
})
}).should('equal', undefined)
})

it('yields undefined if an object contains a symbol', () => {
cy.switchToDomain('foobar.com', () => {
cy.wrap({
key: Symbol('whoops'),
})
}).should('equal', undefined)
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' }?

it('yields an object containing valid types', () => {
cy.switchToDomain('foobar.com', () => {
cy.wrap({
array: [
1,
2,
],
bool: true,
null: null,
number: 12,
object: {
key: 'key',
},
string: 'string',
})
}).should('deep.equal', {
array: [
1,
2,
],
bool: true,
null: null,
number: 12,
object: {
key: 'key',
},
string: 'string',
})
})
})
})
7 changes: 5 additions & 2 deletions packages/driver/src/cy/multi-domain/commands_manager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { PrimaryDomainCommunicator } from '../../multi-domain/communicator'
import { createDeferred, Deferred } from '../../util/deferred'
import { correctStackForCrossDomainError } from './util'
import { deserialize } from '../../multi-domain/serializer'

export class CommandsManager {
// these are proxy commands that represent real commands in a
Expand Down Expand Up @@ -57,15 +58,17 @@ export class CommandsManager {
Cypress.action('cy:enqueue:command', attrs)
}

endCommand = ({ id, name, err, logId }) => {
endCommand = ({ id, subject, name, err, logId }) => {
const command = this.commands[id]

if (!command) return

delete this.commands[id]

if (!err) {
return command.deferred.resolve()
const deserializedSubject = deserialize(subject)

return command.deferred.resolve(deserializedSubject)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// If the command has failed, cast the error back to a proper Error object
Expand Down
11 changes: 8 additions & 3 deletions packages/driver/src/cy/multi-domain/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import $errUtils from '../../cypress/error_utils'
import { CommandsManager } from './commands_manager'
import { LogsManager } from './logs_manager'
import { Validator } from './validator'
import { deserialize } from '../../multi-domain/serializer'

export function addCommands (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy, state: Cypress.State, config: Cypress.InternalConfig) {
let timeoutId
Expand Down Expand Up @@ -118,7 +119,7 @@ export function addCommands (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy,
logsManager.listen()

return new Bluebird((resolve, reject) => {
communicator.once('ran:domain:fn', (err) => {
communicator.once('ran:domain:fn', ({ subject, err }) => {
sendReadyForDomain()
if (err) {
if (done) {
Expand All @@ -141,9 +142,13 @@ export function addCommands (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy,
// is common if there are only assertions enqueued in the SD.
if (!commandsManager.hasCommands && !done) {
cleanup()
}
// This handles when a subject is returned synchronously
const deserializedSubject = deserialize(subject)

resolve()
resolve(deserializedSubject)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
resolve()
}
})

// If done is NOT passed into switchToDomain, wait for the command queue
Expand Down
2 changes: 1 addition & 1 deletion packages/driver/src/cypress/log.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const toSerializedJSON = function (attrs) {
return value()
}

if (_.isFunction(value)) {
if (_.isFunction(value) || _.isSymbol(value)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return value.toString()
}

Expand Down
7 changes: 6 additions & 1 deletion packages/driver/src/multi-domain/commands.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { $Cy } from '../cypress/cy'
import type { SpecBridgeDomainCommunicator } from './communicator'
import { serialize } from './serializer'

export const handleCommands = (Cypress: Cypress.Cypress, cy: $Cy, specBridgeCommunicator: SpecBridgeDomainCommunicator) => {
const onCommandEnqueued = (commandAttrs: Cypress.EnqueuedCommand) => {
Expand All @@ -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'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// we need to serialize and send back the subject on each command because the next chained
// command outside of the multi-domain context will not wait for the queue finished event.
specBridgeCommunicator.toPrimary('command:end', { id, name, subject: serializedSubject })
}

const onRunCommand = () => {
Expand Down
7 changes: 4 additions & 3 deletions packages/driver/src/multi-domain/domain_fn.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { $Cy } from '../cypress/cy'
import type { SpecBridgeDomainCommunicator } from './communicator'
import { serialize } from './serializer'

export const handleDomainFn = (cy: $Cy, specBridgeCommunicator: SpecBridgeDomainCommunicator) => {
const doneEarly = () => {
Expand Down Expand Up @@ -86,11 +87,11 @@ export const handleDomainFn = (cy: $Cy, specBridgeCommunicator: SpecBridgeDomain
// await the eval func, whether it is a promise or not
// we should not need to transpile this as our target browsers support async/await
// see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function for more details
await window.eval(fnWrapper)(data)
const subject = await window.eval(fnWrapper)(data)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} catch (err) {
specBridgeCommunicator.toPrimary('ran:domain:fn', err)
specBridgeCommunicator.toPrimary('ran:domain:fn', { err })
} finally {
cy.state('done', undefined)
}
Expand Down
44 changes: 44 additions & 0 deletions packages/driver/src/multi-domain/serializer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { isFunction, isSymbol } from 'lodash'
import debugFn from 'debug'

const debug = debugFn('cypress:driver:cypress')

/**
* serialize. Used for serializing subject to be returned from subdomains to the primary domain.
* @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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 => {

let serializedSubject: string = undefined!

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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also might need to add isError here as well.

throw 'abort serialization'
}

return value
})
} catch (error) {
// we only want to silence serialization errors.
if (error === 'abort serialization') {
debug(`Serialization aborted for subject: ${value}`)
} else {
throw error
}
}

return serializedSubject
}

/**
* deserialize. Companion to the above serialize function.
* @param value a string
* @returns the deserialized object
*/
const deserialize = (value: string|undefined): string|object|boolean|number|null|any[] => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return value ? JSON.parse(value) : value
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

export { serialize, deserialize }