chore: Update switchToDomain signature with args key
#20722
Conversation
|
Thanks for taking the time to open a PR!
|
AtofStryker
changed the title
args key
AtofStryker
changed the title
args keyargs key
AtofStryker
force-pushed
the
update_switch_to_domain_signature
branch
from
a017acb
to
bfc574c
Compare
|
Test summary
Run details
View run in Cypress Dashboard ➡️ 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 |
…erface and exclude user serialized data
There was a problem hiding this comment.
I didn't leave code suggestions everywhere they're needed, but I think we should make sure the argument to switchToDomain is called options and that we consistently use args instead of data for the object getting serialized and passed to the switchToDomain callback.
| } else if (options && !Object.getOwnPropertyDescriptor(options, 'args')) { | ||
| this.onFailure() | ||
|
|
||
| $errUtils.throwErrByPath('switchToDomain.incomplete_options_argument', { | ||
| onFail: this.log, | ||
| args: { arg: $utils.stringify(options) }, |
There was a problem hiding this comment.
I don't think we should throw in this case. It's fine for there to be an empty options object and in the future, it will be fine for it only have other options and not the args property.
There was a problem hiding this comment.
I figured since we do not have any additional properties as of now, it would make sense to verify that the args key exists, and then update this method and types as we add in other options. Currently, throwing here might be beneficial to the user so they don't try to do something like below and then scratch their head for a while trying to figure out what was missed.
cy.switchToDomain('foobar.com', { foo: 'foo' }, ({ foo }) => {
expect(foo).to.be('foo') // fails because it's undefined
})
There was a problem hiding this comment.
Could we throw if they provide any keys other than the ones we allow? We'd only allow args for now, but that would cover the case where they are a passing in the wrong args at the wrong level.
There was a problem hiding this comment.
I like @mjhenkes's idea.
There was a problem hiding this comment.
@mjhenkes I'll update the PR to favor checking for keys other than that which we allow. I'll probably opt for something similar to how we verify the cypress config
There was a problem hiding this comment.
updated in 2f8317f
…in options argument over missing the args key
…s-io/cypress into update_switch_to_domain_signature
User facing changelog
n/a
Additional details
The goal of this PR is to update the
dataargument inswitchToDomainto be an object with anargsproperty where users can pass in user defined data to be sent over to theirswitchToDomaincall. This is currently replacing a generic array, which causes issues when there are are multiple types in the array and the index of said type cannot be implied, leading toORon the types in the array.Currently,
argsis required if leveraging thedataargument since there currently aren't other options. aTODOcomment has been added to overload the type once options become available, which will give better typings toargsin theswitchToDomaincallback itself.How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?cypress.schema.json?