-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Add 'type' option to .as
to store aliases by value
#25251
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
@@ -42,7 +58,7 @@ export default function (Commands, Cypress, cy) { | |||
|
|||
if (!alreadyAliasedLog && log) { | |||
log.set({ | |||
alias, | |||
alias: `@${alias}${options.type ? ` (${ options.type })` : ''}`, |
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.
alias: `@${alias}${options.type ? ` (${ options.type })` : ''}`, | |
alias: `@${alias}${options.type ? ` (${ options.type })` : '(query)'}`, |
the default is type query, so why not set this as the fallback?
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.
Take a look at the "After" screenshot it the PR description, showing how it looks with (value)
tagged onto the end - it's a bit visually busy, so I was trying to avoid it in the default case. If the user sets it explicitly - whether it be query or value - then it'll show up in the command log.
Can definitely make it always display if we like, just 'display less by default' was my intention here.
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.
@BlueWinds Because these wouldn't align
.as('name')
.as('name', { type: 'query' }
Could we only prepend when the non-default is used (i.e. only when type === value)?
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'd be helpful to also add this type to the dev tools log details when clicking a log that is referencing an alias to provide the in-depth context to how the alias value is resolved.
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.
Changed it to be consistent, always displaying value
and never displaying query
; see updated screenshot in the PR description.
I'm going to push back against adding it to the console props for .get() when an alias is retrieved - the "type" of an alias is a property of how it's stored, not how it's retrieved, so we should display it where it's stored (which we do).
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.
The documentation PR says the type
impacts how the value is retrieved later in the test. 😅
|
||
export default function (Commands, Cypress, cy) { | ||
Commands.addQuery('as', function asFn (alias) { | ||
Commands.addQuery('as', function asFn (alias, options = {} as Partial<Cypress.AsOptions>) { |
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'd expect to use Cypress.AsOptions directly . Should be able to if the keys are optional.
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 opted here for consistency. Pre-existing commands all use Partial<>
in this context, and do not have optional properties.
as(alias: string, options?: Partial<AsOptions>): Chainable<Subject>
blur(options?: Partial<BlurOptions>): Chainable<Subject>
check(options?: Partial<CheckOptions>): Chainable<Subject>
...snip...
interface CheckOptions extends Loggable, Timeoutable, ActionableOptions {
interval: number
}
Removed the optional-ness of AsOptions.type
it , since we have Partial<> everywhere AsOptions is used.
cli/types/cypress.d.ts
Outdated
*/ | ||
action: 'select' | 'drag-drop' | ||
type?: 'query' | 'value' |
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.
Pulling comment forward from the docs - worth getting other opinions on I think.
These types seem very specific to element queries and context sharing. The
query
type seems like an odd name for aliasing requests or intercepts since they aren't query commands, but these types of aliases would likely need to default to query to work as expected.
Have we considered alternatives like dynamic
and static
or other names that can encompass/reflect non-query command aliases as well?
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.
static
does read nicer than value
, switching over to that. Will update docs PR shortly. I do prefer query
for the dynamic / default version though - "query" is a cypress concept we reference in the docs, and we should be consistent about its usage.
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.
static as default please
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.
Defaulting to static
would be a breaking change for everyone aliasing DOM nodes, which is more common than aliasing static values. Reducing or eliminating detached DOM errors was the motivation for making this change to alias behavior in the first place.
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.
Humm ok, got it. Thank you!
Co-authored-by: Chris Breiding <[email protected]>
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'd be nice if the types description matched the documentation content.
Co-authored-by: Emily Rohrbough <[email protected]>
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
.as()
now accepts anoptions
argument, allowing test writers to store an alias as a "query" or "static".A "query" alias (the default) re-quries the application for the current state whenever retrieved - it's a 'live' value.
A "static" alias is stored once, when the alias is created - it's a static value, and will never update.
Don't store DOM references as 'value's.
Additional details
Steps to test
How has the user experience changed?
Before:
After:
Aliases are
query
by default. If the user sets thetype
tostatic
, then it is included in the command log.PR Tasks
cypress-documentation
? Add 'type' option for .as() command; Improve visibility of alias changes in 12.x cypress-documentation#4943type definitions
?