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

Queries, Detached DOM, and Retry-Ability #4835

Merged
merged 10 commits into from
Nov 28, 2022

Conversation

BlueWinds
Copy link
Contributor

@BlueWinds BlueWinds commented Nov 3, 2022

Associated code PR: cypress-io/cypress#24628 into the release/12.0.0 branch.

This PR updates the Cypress documentation for the changes coming in Cypress 12 around the addition of "Queries" to Cypress, as a means to address cypress-io/cypress#7306, aka Detached DOM.

For reviewing this, I suggest reading the new version of retry-ability in its entirety first, and commenting on that. Then go back and look at the diff from the old one, to see if there's anything that was left out. Comparing them section-by-section is probably not all that fruitful.

@vercel
Copy link

vercel bot commented Nov 3, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cypress-documentation ✅ Ready (Inspect) Visit Preview Nov 15, 2022 at 8:35PM (UTC)

@BlueWinds BlueWinds changed the title First rework of retryability guide Queries, Detached DOM, and Retry-Ability Nov 8, 2022
@@ -52,8 +52,9 @@ Pass in an options object to change the default behavior of `.clear()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`.clear()` yields the same subject it was given from the previous
command.</li></List>
- `.clear()` yields the same subject it was given.
Copy link
Member

Choose a reason for hiding this comment

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

why drop from the previous command. from all of the API docs?

Suggested change
- `.clear()` yields the same subject it was given.
- `.clear()` yields the same subject it was given from the previous command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"From the previous command, query or assertion" is pretty verbose, and I don't think it actually adds any clarity. Assertions usually don't change the subject, but they can! So even before queries, this wasn't quite accurate.

@@ -32,7 +32,8 @@ Pass in an options object to change the default behavior of `cy.document()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.document()` 'yields the `window.document` object' </li></List>
- `cy.document()` 'yields the `window.document` object.
- `cy.document()` is a query, and it is _safe_ to chain further methods.
Copy link
Member

Choose a reason for hiding this comment

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

@BlueWinds Should all Query commands be moved to live under APIs/queries to help alleviate confusion when a user is looking at the docs api face up?

Copy link
Member

@emilyrohrbough emilyrohrbough Nov 8, 2022

Choose a reason for hiding this comment

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

It might also provide another (or better) face up opportunity to describe the difference between query & command and how to add custom and the retry-ability differences are (at this point only through the upper docs so you might have done this...) It'd live closer to the commands/queries at least,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean changing the URL, so that it's /apis/query/document rather than /api/command/document? I'm definitely open to the idea - we'd need to leave in place redirects so that we don't break links, but that's not hard.

I'm not sure I follow the second comment though. Can you explain what you mean again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think having all the methods on a single page is much more convenient - just having the "... is a query ..." label (and maybe it linking to the location where we describe the differences) seems good to me.

Don't feel really strongly about it, but I think the majority of users won't think about whether something is a query/command too often, they will just try something and let the error messages guide them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the slug wouldn't necessarily remove them from the same page - you'd still see them in the TOC.

Screenshot_20221109_073631

/api/commands/..., /api/utilities/... and /api/plugins/... are already there.

@elylucas - if I wanted to move /api/commands/as to /api/queries/as, and have /api/commands/as redirect to the new URL, how would I go about this?

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 it's a good idea to put more clarification around this as it is starting to become more important in the distinction between queries/commands/assertions. It will also help people understand whats happening when based on the type of method they are calling.

@BlueWinds If you wanted to update the directory structure now you would rename the files into their new location (make sure git sees it as a rename), update sidebar.json with the new paths, and add redirects to the netflify.toml file for the new urls. We'd need to check the onlinks app in the services monorepo and see if any onlinks need to be updated.

I'm fine if you want to do this now or wait until we convert this content over to docusaurus as it will be a requirement there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't onlinks continue to work, if we add a redirect inside the docs? Eg, on.cypress.io/get -> docs.cypress.io/api/commands/get -> docs.cypress.io/api/queries/get?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't onlinks continue to work, if we add a redirect inside the docs? Eg, on.cypress.io/get -> docs.cypress.io/api/commands/get -> docs.cypress.io/api/queries/get?

They would, but good to update to keep the double redirect from happening and to be correct in the services repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-organized the table of contents following discussions with Brian and Emily. We now go:

Commands

  • Assertions
  • Actions
  • Queries

and then all the other commands that aren't one of these three beneath "Commands". I've added links to relevant guides to each section.

With this hierarchy change, I'm not sure it makes sense to update the slugs any more - they're all still "Commands", some of them just also have additional modifiers ("query command", "action command").

@@ -40,8 +40,7 @@ Pass in an options object to change the default behavior of `cy.clearCookies()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.clearCookies()` yields `null`.</li><li>`cy.clearCookies()` cannot
be chained further.</li></List>
- `cy.clearCookies()` yields `null`.
Copy link
Member

Choose a reason for hiding this comment

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

Is clearCookies a query or a command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Command. Also not sure it needs to be called out in the 'Yields' header though - it yields null, that's kind of the end of it. 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

curiosity question, if it yields null how can you continue to chain off it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Showed an example in a comment here - #4835 (comment)

The problem is that "parent" and "child" commands do not work the way you expect them to from the name.

  • Parent commands are those that don't care about their subject - being a parent does not limit where in a chain a command can be!
  • Dual commands accept both null and a valid subject. They behave differently based on what subject they're given.
  • Child commands require a subject. But this is a rule about the subject they're passed, not about their position. cy.end().click() is invalid because the subject is null, not because .end() can't be chained off of.

This is all existing behavior from Cy1-11, covered by our own tests - the docs have just never explained it clearly.

Copy link
Contributor Author

@BlueWinds BlueWinds Nov 10, 2022

Choose a reason for hiding this comment

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

  cy.visit('/fixtures/dom.html')
    .clearCookies('foo')
    .clearCookies('foo')
    .clearCookies('foo')
    .get('form')
    .get('form')
    .get('form')

Seven parent commands chained together. Our own tests do this all the time, as do our example repos. Forcing parents to actually be parents or disallowing chaining after a command that returns null would force people to rewrite tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we use the term "yields" and not "returns" is because it's stating which subject is passed to the next command, not what is returned from the command. All commands return a chainer object that allows chaining further commands.

While we do indeed use a lot of unnecessary chaining in our tests, there's at least one legitimate use for chaining after a command that yields null:

cy.clearCookies().then(() => {
  // do something after clearing the cookies has completed
})

@@ -26,15 +35,19 @@ subject, use [`.its()`](/api/commands/its).
**<Icon name="check-circle" color="green"></Icon> Correct Usage**

```javascript
cy.wrap({ animate: fn }).invoke('animate') // Invoke the 'animate' function
cy.get('.input').invoke('val').should('eq', 'foo') // Invoke the 'val' function
Copy link
Member

Choose a reason for hiding this comment

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

given the alert above, wouldn't invoke('val) run 1-many times until should is satisfied or it times out? Is this the expected usage of invoke?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the expected usage of invoke, and one of the most common ways people use it (as measured via querying metabase test bodies).

"Get an input, invoke the val method on the jquery object, assert that the value matches a string" is 👍

```

**<Icon name="exclamation-triangle" color="red"></Icon> Incorrect Usage**

```javascript
cy.invoke('convert') // Errors, cannot be chained off 'cy'
cy.wrap({ name: 'Jane' }).invoke('name') // Errors, 'name' is not a function
cy.wrap({ animate: fn })
.invoke('animate')
.then(() => {}) // 'animate' will be called multiple times
Copy link
Member

Choose a reason for hiding this comment

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

.then() is one of the few commands that doesn't actually doesn't retry (unless that has changed) so we might want to update this example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually switched to .then() exactly for this reason - even if the following command doesn't retry and doesn't care about the subject, .invoke() will still get called twice - once when executing it, and then a second time when determining the subject for .then().

Copy link
Member

@emilyrohrbough emilyrohrbough left a comment

Choose a reason for hiding this comment

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

I'm partially through. Will need to continue later.

content/api/commands/reload.md Outdated Show resolved Hide resolved
content/api/commands/root.md Outdated Show resolved Hide resolved
<List><li>`.scrollIntoView()` yields the same subject it was given from the
previous command.</li></List>
- `.scrollIntoView()` yields the same subject it was given.
- `.scrollIntoView()` is a command, and it is **unsafe** to chain further
Copy link
Member

Choose a reason for hiding this comment

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

Is scrollIntoView and scrollTo actually unsafe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This was the source of some of our test flake - when list virtualization is involved, scrolling causes a great many DOM rerenders and updates, and the original element you wanted to scroll into vue (view 😁 ) may be replaced.


`cy.spy()` returns a [Sinon.js spy](https://sinonjs.org/releases/v6.1.5/spies/).
All methods found on Sinon.JS spies are supported.
- `cy.spy()` is a _utility function_, and is neither a command nor a query.
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean in terms of retry-ability & chainability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.spy() and .stub() are super weird cases - they really belong under the Utilities header and slug, and should not be using the "command" template at all, pretending things like 'assertions', 'timeouts' and 'yields' apply to them.

They don't retry - they don't even enter the command queue. They cannot be chained at all. cy.spy().as() doesn't even use the .as() command - it uses a custom function, and doesn't return a $Chainer.

I'll try and update the docs to be clearer about this.

a `.then()` interface) and Cypress will wait for that to resolve before
continuing forward through the chain of commands.
flow into the next command (with the exception of `undefined` or `null`). If the
return value is a Promise or other "thenable" (anything with a `.then()`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return value is a Promise or other "thenable" (anything with a `.then()`
return value is a Promise or a Chained `.then()` command

doesn't it way for any returned cypress command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not actually what the docs are trying to say - it means literally "if the return value is a promise or something like return { then: () => {}}."

With that said, I do think it is extremely unclear, and am updating the wording to be more straightforward.

content/api/commands/then.md Outdated Show resolved Hide resolved
content/api/commands/then.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

These look good. The most interesting file is hidden by GH by default: https://github.com/cypress-io/cypress-documentation/pull/4835/files#diff-4d501082dd1739f23f2ed58152f22a6747b15cdc3b77f830a86226c8f3b5ac55

I hope the distinction between queries/commands will help make things more clear to users. It kind of already existed implicitly, now it's more clear and defined, which is great.

@@ -32,7 +32,8 @@ Pass in an options object to change the default behavior of `cy.document()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.document()` 'yields the `window.document` object' </li></List>
- `cy.document()` 'yields the `window.document` object.
- `cy.document()` is a query, and it is _safe_ to chain further methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think having all the methods on a single page is much more convenient - just having the "... is a query ..." label (and maybe it linking to the location where we describe the differences) seems good to me.

Don't feel really strongly about it, but I think the majority of users won't think about whether something is a query/command too often, they will just try something and let the error messages guide them.

There's nothing special about these functions - they simply validate their
argument and throw an error if the check fails. You can throw errors of any type
at any time inside your queries - Cypress will catch and handle it
appropriately.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great guide 👍 I think a lot of internal people would also benefit from reading it, until I read followed the detached DOM PRs I didn't fully grok how the driver worked, but now I do and reading this makes it even more clear.

<List><li>`.as()` yields the same subject it was given from the previous
command.</li></List>
- `.as()` yields the same subject it was given.
- `.as()` is a query, and it _safe_ to chain further methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

how is as() a query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a query because it's implemented with Commands.addQuery(), not Commands.add(). It's implemented this way so that it's safe to chain off of.

cy.get('button').as('btn').click()

is something we want to support, so .as() is a query. Basically, if something can be a query, it should be, to give our users maximum safety and freedom to string together methods as they please.

I think that's the basic intuition I'm having trouble conveying: "Queryness" is about how methods chain together, not about the DOM. Very open to a better name, if you have any suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

there seems to be a number of "utility" functions that fall into scenario. I think calling them queries is confusing. Maybe we can refer to them as utilities (also open to better naming), and call out they are chainable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Utility is heavily overloaded already - we have 'utility commands', and a 'Utilities' section in the API docs.

How about just removing the word "query" here?

- It is _safe_ to chain further methods after `.as()`.

Copy link
Contributor Author

@BlueWinds BlueWinds Nov 10, 2022

Choose a reason for hiding this comment

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

The three sticking points for the query name seem to be:

  • .as()
  • .invoke()
  • .its()

I'd argue that .invoke() and .its() fit very well.

"I to know this button's title" -> cy.get('button').invoke('attr', 'title')
"I want to know the value of this form" -> cy.get('form').invoke('val')
"I want to know the request body" -> cy.get('@myRequest.1').its('req.body')
"I want to know the user's name" -> cy.wrap(userObj).its('name.first')

All of these are very naturally thought of as "queries". In SQL, you query the database. In CSS, you query the DOM. In Cypress, you query your application - Cypress allows subjects other than DOM elements, so you should be able to query things other than DOM elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed in a meeting an decided that we'd change things around a bit into a hierarchy:

Commands:

  • Queries
  • Actions
  • Assertions
  • ...other commands...

I'm updating the PR to reflect these changes.

@@ -40,8 +40,7 @@ Pass in an options object to change the default behavior of `cy.clearCookies()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.clearCookies()` yields `null`.</li><li>`cy.clearCookies()` cannot
be chained further.</li></List>
- `cy.clearCookies()` yields `null`.
Copy link
Contributor

Choose a reason for hiding this comment

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

curiosity question, if it yields null how can you continue to chain off it?

@@ -32,7 +32,8 @@ Pass in an options object to change the default behavior of `cy.document()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.document()` 'yields the `window.document` object' </li></List>
- `cy.document()` 'yields the `window.document` object.
- `cy.document()` is a query, and it is _safe_ to chain further methods.
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 it's a good idea to put more clarification around this as it is starting to become more important in the distinction between queries/commands/assertions. It will also help people understand whats happening when based on the type of method they are calling.

@BlueWinds If you wanted to update the directory structure now you would rename the files into their new location (make sure git sees it as a rename), update sidebar.json with the new paths, and add redirects to the netflify.toml file for the new urls. We'd need to check the onlinks app in the services monorepo and see if any onlinks need to be updated.

I'm fine if you want to do this now or wait until we convert this content over to docusaurus as it will be a requirement there.

content/api/commands/end.md Show resolved Hide resolved
content/api/commands/invoke.md Outdated Show resolved Hide resolved
content/api/commands/spread.md Outdated Show resolved Hide resolved
- `cy.spy()` is _synchronous_ and returns a value (the spy) instead of a
Promise-like chain-able object. It can be aliased.
- `cy.spy()` returns a
[Sinon.js spy](https://sinonjs.org/releases/v6.1.5/spies/). All methods found
Copy link
Contributor

Choose a reason for hiding this comment

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

could also link to /guides/guides/stubs-spies-and-clocks#Spies if we didn't want to link to an external resource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this would be less useful than the current link - that section basically just points users straight back to this page, without any details about the spy API.

Adding an alias using [`.as()`](/api/commands/as) to spies makes them easier to
identify in error messages and Cypress' command log.
You can alias spies, similar to how [`.as()`](/api/commands/as) works. This can
make your spies easier to identify in error messages and Cypress's command log.
Copy link
Contributor

Choose a reason for hiding this comment

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

its also how you would get the spy later to assert against

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this line, and also the example below to show that usage.

content/api/cypress-api/custom-commands.md Outdated Show resolved Hide resolved
<List><li>`.as()` yields the same subject it was given from the previous
command.</li></List>
- `.as()` yields the same subject it was given.
- `.as()` is a query, and it _safe_ to chain further methods.
Copy link
Contributor

Choose a reason for hiding this comment

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

there seems to be a number of "utility" functions that fall into scenario. I think calling them queries is confusing. Maybe we can refer to them as utilities (also open to better naming), and call out they are chainable?

Comment on lines 419 to 427
<Alert type="warning">

`Cypress.Commands.overwrite` can only overwrite commands, not queries. If you
want to modify the behavior of a query, you'll need to use
[`Cypress.Commands.overwriteQuery`](/api/cypress-api/custom-queries#overwriting-existing-queries)
instead.

</Alert>

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
<Alert type="warning">
`Cypress.Commands.overwrite` can only overwrite commands, not queries. If you
want to modify the behavior of a query, you'll need to use
[`Cypress.Commands.overwriteQuery`](/api/cypress-api/custom-queries#overwriting-existing-queries)
instead.
</Alert>

Not sure if this should be completely removed or if maybe there's some other advice we can offer if they're trying to overwrite a query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to offer new advice about how to customize queries.

`Cypress.Commands.overwrite` can only overwrite commands, not queries. If you
want to modify the behavior of a query, you'll need to add your updated version
under a new name using
[`Cypress.Commands.addQuery`](/api/cypress-api/custom-queries) instead.

content/api/cypress-api/custom-queries.md Outdated Show resolved Hide resolved
title: Custom Queries
---

Cypress comes with its own API for creating custom queries. The built in Cypress
Copy link
Contributor

Choose a reason for hiding this comment

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

should we call out that this is only true since v12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, added.

content/api/cypress-api/custom-queries.md Outdated Show resolved Hide resolved
content/api/cypress-api/custom-commands.md Outdated Show resolved Hide resolved
2. Queries are _retrieable._ Once you return the inner function, Cypress takes
control, handling retries on your behalf.
3. Queries are _idempotent._ Once you return the inner function, Cypress will
invoke it repeatedly.
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
invoke it repeatedly.
invoke it repeatedly. Invoking the inner function multiple times will not mutate the DOM.

Maybe something like this to call out what we mean by idempotent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with Invoking the inner function multiple times must not change the state of your application., to make it clear that this isn't just about the DOM, it's about the whole application. Reseeding the database or resetting cookies are also not queries.

content/api/cypress-api/custom-queries.md Outdated Show resolved Hide resolved
content/api/cypress-api/custom-queries.md Outdated Show resolved Hide resolved
content/api/cypress-api/custom-queries.md Outdated Show resolved Hide resolved

```javascript
Cypress.Commands.addQuery('getFirstButton', function getFirstButton(options) {
const getFn = cy.now('get', 'button:first', options)
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 need to use cy.now() here? Why wouldn't we just use cy.get()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see its explained a bit below but I still don't quite understand it. It also causes errors in TS as theres no definition for cy.now() (unless that is new in your branch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. We do this several times in our own code - I'm not sure why it's a TS error. cy.now() has always existed. Will look into the TS error in a moment here.

Added another paragraph trying to explain this further.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we've expected cy.now() to users...it's essentially sync Cypress commands vs queuing a cypress command.

## See also

- See how to add
[TypeScript support for custom commands](/guides/tooling/typescript-support#Types-for-custom-commands)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there anything different that needs to be done to set up types for queries vs commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, they are the same from a TypeScript perspective.

content/guides/component-testing/slots-vue.md Outdated Show resolved Hide resolved
@@ -80,8 +80,7 @@ Pass in an options object to change the default behavior of `cy.viewport()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.viewport()` yields `null`.</li><li>`cy.viewport()` cannot be
chained further.</li></List>
- `cy.viewport()` yields `null`.
Copy link
Member

Choose a reason for hiding this comment

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

command or query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cy.viewport() is used to set the viewport size, not query it. It's closer to cy.visit() than it is cy.get().

I don't think it makes sense to say "viewport is not an action command", and for much the same reason I don't think it makes sense to call out "viewport is not a query command."

content/api/commands/wait.md Outdated Show resolved Hide resolved
@@ -37,7 +37,7 @@ Pass in an options object to change the default behavior of `cy.wrap()`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`cy.wrap()` 'yields the object it was called with' </li></List>
- `cy.wrap()` yields the object it was called with.
Copy link
Member

Choose a reason for hiding this comment

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

utility command?this is likely unsafe to chain if the subject is a dom el?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unsafe warning is meant to be "here's how to avoid a common pitfall, detached DOM errors", and I added it to commands that yield DOM elements.

It could potentially be unsafe to do something like cy.wrap(cy.$$('button')), but if people are doing that, then they need to go re-read "Introduction to Cypress", not a specific warning on the cy.wrap() "yields" section.

@@ -49,8 +52,9 @@ Pass in an options object to change the default behavior of `.blur`.

### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management)

<List><li>`.blur()` yields the same subject it was given from the previous
command.</li></List>
- `.blur()` yields the same subject it was given.
Copy link
Member

Choose a reason for hiding this comment

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

quick preview pass through - this should be an action command correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.cypress.io/api/commands/blur#Actionability

Blur is not an action command

.blur() is not implemented like other action commands, and does not follow the same rules of waiting for actionability.

.blur() is a helpful command used as a shortcut. Normally there's no way for a user to "blur" an element. Typically the user would have to perform another action like clicking or tabbing to a different element. Needing to perform a separate action like this is very indirect.
Therefore it's often much more efficient to test the blur behavior directly with .blur().

@@ -4,6 +4,9 @@ title: focus

Focus on a DOM element.

It is [unsafe](/guides/retry-ability#Only-Queries-are-retried) to chain further
Copy link
Member

Choose a reason for hiding this comment

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

action command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was pretty surprised by this too, not particularly intuitive. 🤷‍♀️

Focus is not an action command

.focus() is not implemented like other action commands, and does not follow the same rules of waiting for actionability.

.focus() is a helpful command used as a shortcut. Normally there's no way for a user to "focus" an element without causing another action or side effect. Typically the user would have to click or tab to this element.

content/api/cypress-api/custom-queries.md Outdated Show resolved Hide resolved
instance. See [`Cypress.log()`](/api/cypress-api/cypress-log) for more
information.

This is setup code - we only want it to run once, creating the log message when
Copy link
Member

Choose a reason for hiding this comment

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

can we call this outer function or change the above outer func reference to setup code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

This line is setup code, so it lives in the outer function - we only want it
to run once, creating the log message when Cypress first begins executing this
query. We hold onto a reference to Log instance. We'll update it later with
additional details when the inner function executes.

information.

This is setup code - we only want it to run once, creating the log message when
Cypress first begins executing this query. We hold onto a reference to `Log`
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean these log references always exist until a test has completed due to the command chain re-executing the inner function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what you're getting at? Yes, the closure holds onto a reference to the $Log object, so as long as the closure is referenced, $Log can't be GCed.

Existing commands do the exact same thing, though they mostly store their reference as $Command.attributes._log, rather than in a closure. Changing the place we hold onto a reference (as a closure local variable rather than as an attribute on an object) shouldn't have any memory-usage implications.


```javascript
Cypress.Commands.addQuery('getFirstButton', function getFirstButton(options) {
const getFn = cy.now('get', 'button:first', options)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we've expected cy.now() to users...it's essentially sync Cypress commands vs queuing a cypress command.

Comment on lines +310 to +326
- `cy.ensureSubjectByType(subject, types, this)`: Accepts an array with any of
the strings `optional`, `element`, `document`, or `window`.
`ensureSubjectByType` is how
[`prevSubject` validation](/api/cypress-api/custom-commands#Validations) is
implemented for commmands.
- `cy.ensureElement(subject, queryName)`: Ensure that the passed in `subject` is
one or more DOM elements.
- `cy.ensureWindow(subject)`: Ensure that the passed in `subject` is a
`document`.
- `cy.ensureDocument(subject)`: Ensure that the passed in `subject` is a
`window`.

- `cy.ensureAttached(subject, queryName)`: Ensure that DOM element(s) are
attached to the page.
- `cy.ensureNotDisabled(subject)`: Ensure that form elements aren't disabled.
- `cy.ensureVisibility(subject)`: Ensure that a DOM element is visible on the
page.
Copy link
Member

Choose a reason for hiding this comment

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

SOOO, we this exposes internal, sync APIs attached to cy. We have communicated methods associated/chained off of cy are queueable and do not execute right away, where as APIs chained off of Cypress will execute immediate. I wonder if this will be confusing to users on why these aren't exposed as Cypress APIs.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, none of these have Public Typescript types. We should get an issue logged to type/expose these, even if it's post GA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I don't really think these belong on cy itself, but they've been there for years and I'm not sure I want to move them as part of this release. :/

I'll include adding TS types for these methods as part of my next code PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cypress-io/cypress#24697 - will move this out of draft once tests pass.

@@ -4,6 +4,9 @@ title: focus

Focus on a DOM element.

It is [unsafe](/guides/retry-ability#Only-Queries-are-retried) to chain further
commands that rely on the subject after `.focus()`.
Copy link
Member

Choose a reason for hiding this comment

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

This contradicts our current recommendation for .blur() by saying this is unsafe to chain further...

This element must currently be in focus. If you want to ensure an element is
focused before blurring, try using .focus() before
.blur().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the recommendation in .blur() to make it clearer.

This element must currently be in focus. If you want to ensure an element is
focused before blurring, try using .focus() before
.blur().

cy.get('button').as('btn').focus()
cy.get('@btn').blur()

@elylucas elylucas merged commit 5501639 into v12 Nov 28, 2022
@elylucas elylucas deleted the issue-7306-addQuery-actionability branch November 28, 2022 20:06
debrisapron pushed a commit that referenced this pull request Dec 6, 2022
* First rework of retryability guide

* Update each command's Yields section, and all guides, with information about queries vs commands

* Add Custom Queries page

* Minor formatting tweaks

* Review changes

* Review updates

* Update based on review + last week meetings

* More review updates

* Fix tests
mjhenkes added a commit that referenced this pull request Dec 6, 2022
* Remove pages and references to functionality obsoleted by multidomain GA

* fix: Explain error thrown when cypress commands in .should() callback (#4755)

* fix: Explain error thrown when cypress commands in .should() callback

* Improve layout of previous changes and provide second example of how to fix

* Update content/api/commands/should.md

Co-authored-by: Rachel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Zach Bloomquist <[email protected]>

* Run prettier

* Run prettier again...?

* One more prettier run... :/

Co-authored-by: Rachel <[email protected]>
Co-authored-by: Zach Bloomquist <[email protected]>

* docs: removing Cookies.defaults/preserveOnce (#4779)

* docs: remove experimentalSessionAndOrigin (#4807)

* Update cookie commands domain option description (#4861)

* docs: Queries, Detached DOM, and Retry-Ability (#4835)

* First rework of retryability guide

* Update each command's Yields section, and all guides, with information about queries vs commands

* Add Custom Queries page

* Minor formatting tweaks

* Review changes

* Review updates

* Update based on review + last week meetings

* More review updates

* Fix tests

* breaking: drop node 12, 13, 15 and 17 support (#4879)

* Add docs for new local/session storage commands (#4876)

* feat: update okta login guide for realworld app (#4883)

* feat: update okta login guide for realworld app

* chore: make changes to okta to have parity with cognito changes

* chore: address code review comments

* feat: update cognito login guide for realworld app (#4882)

* feat: update cognito login guide for realworld app

* chore: update guide from comments in code review

* properly close alert tag

* Update content/guides/end-to-end-testing/amazon-cognito-authentication.md

* chore: address comments from code review

* fix linting

* v12 Migration Guide (#4862)

Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Blue F <[email protected]>
Co-authored-by: DEBRIS APRON <[email protected]>
Co-authored-by: Ben M <[email protected]>
Closes undefined

* Small update to cy.origin API docs for v12

* Update auth examples for v12 on custom commands page

* 12: update test isolation docs to use true/false instead of on/off (#4890)

Co-authored-by: Chris Breiding <[email protected]>
Co-authored-by: Matt Henkes <[email protected]>
Co-authored-by: Bill Glesias <[email protected]>

* docs: add documentation for experimentalOriginDependencies (#4897)

* Documentation updates for v12 (#4880)

* re-add websecurity, links to websecurity, and trade-offs guides

* chore: revamp documentation around web security page

* chore: update same-origin tradeoff to be new navigation rules, including our SD chart, to help paint users a clear picture with cy.origin

* chore: link to the experimental modify obstructive third party code doc in web security from origin

* chore: update Error Messages section to reflect allowing cross origin visiting

* update best practices: visiting external sites

* remove node 12 from installing cypress section

* chore: update key differences to plug session and origin over programmatic login

* chore: update with suggestions from code review

* add okta/amazon guide links in trade-offs and update workarounds

* feat: add cross origin testing guide

* update image for command time out with visit

* chore: readd legacy errors and add a Note section to explain that this is only for cypress v11 and under

* chore: update suggestions from code review

* chore: add suggestions from code review

* fix: fix okta alert banner (needed a new line)

* fix: broken image in error messages

* chore: update error header for on link to address cypress-io/cypress-services#5040 (comment)

* Update cy.session API docs for v12 (#4851)

Co-authored-by: Emily Rohrbough <[email protected]>
Closes #4507

* Remove pages and references to functionality obsoleted by multidomain GA

* fix: Explain error thrown when cypress commands in .should() callback (#4755)

* fix: Explain error thrown when cypress commands in .should() callback

* Improve layout of previous changes and provide second example of how to fix

* Update content/api/commands/should.md

Co-authored-by: Rachel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Zach Bloomquist <[email protected]>

* Run prettier

* Run prettier again...?

* One more prettier run... :/

Co-authored-by: Rachel <[email protected]>
Co-authored-by: Zach Bloomquist <[email protected]>

* docs: removing Cookies.defaults/preserveOnce (#4779)

* docs: remove experimentalSessionAndOrigin (#4807)

* Update cookie commands domain option description (#4861)

* docs: Queries, Detached DOM, and Retry-Ability (#4835)

* First rework of retryability guide

* Update each command's Yields section, and all guides, with information about queries vs commands

* Add Custom Queries page

* Minor formatting tweaks

* Review changes

* Review updates

* Update based on review + last week meetings

* More review updates

* Fix tests

* breaking: drop node 12, 13, 15 and 17 support (#4879)

* Add docs for new local/session storage commands (#4876)

* feat: update okta login guide for realworld app (#4883)

* feat: update okta login guide for realworld app

* chore: make changes to okta to have parity with cognito changes

* chore: address code review comments

* feat: update cognito login guide for realworld app (#4882)

* feat: update cognito login guide for realworld app

* chore: update guide from comments in code review

* properly close alert tag

* Update content/guides/end-to-end-testing/amazon-cognito-authentication.md

* chore: address comments from code review

* fix linting

* v12 Migration Guide (#4862)

Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Blue F <[email protected]>
Co-authored-by: DEBRIS APRON <[email protected]>
Co-authored-by: Ben M <[email protected]>
Closes undefined

* 12: update test isolation docs to use true/false instead of on/off (#4890)

Co-authored-by: Chris Breiding <[email protected]>
Co-authored-by: Matt Henkes <[email protected]>
Co-authored-by: Bill Glesias <[email protected]>

* docs: add documentation for experimentalOriginDependencies (#4897)

* Documentation updates for v12 (#4880)

* re-add websecurity, links to websecurity, and trade-offs guides

* chore: revamp documentation around web security page

* chore: update same-origin tradeoff to be new navigation rules, including our SD chart, to help paint users a clear picture with cy.origin

* chore: link to the experimental modify obstructive third party code doc in web security from origin

* chore: update Error Messages section to reflect allowing cross origin visiting

* update best practices: visiting external sites

* remove node 12 from installing cypress section

* chore: update key differences to plug session and origin over programmatic login

* chore: update with suggestions from code review

* add okta/amazon guide links in trade-offs and update workarounds

* feat: add cross origin testing guide

* update image for command time out with visit

* chore: readd legacy errors and add a Note section to explain that this is only for cypress v11 and under

* chore: update suggestions from code review

* chore: add suggestions from code review

* fix: fix okta alert banner (needed a new line)

* fix: broken image in error messages

* chore: update error header for on link to address cypress-io/cypress-services#5040 (comment)

* Update auth examples for v12 on custom commands page

* Small update to cy.origin API docs for v12

* Update cy.session API docs for v12 (#4851)

Co-authored-by: Emily Rohrbough <[email protected]>
Closes #4507

* chore: address docs feedback post merge (#4899)

* .within() now throws an error if given more than one subject (#4898)

* .within() now throws error when passed more than one subject.

* Add migration guide, update based on reviews

* Update Logging In section of Testing Your App page (#4885)

Co-authored-by: Emily Rohrbough <[email protected]>
Closes #4498

* Update End-to-End Testing -> Auth0 Authentication docs for v12 (#4895)

Co-authored-by: Bill Glesias <[email protected]>

* Cypress.Session Cypress API (#4900)

* docs around Cypress.session api

* data not date

* Update content/api/cypress-api/session.md

Co-authored-by: Matt Henkes <[email protected]>

* Update content/api/cypress-api/session.md

Co-authored-by: Matt Henkes <[email protected]>

* Update content/api/cypress-api/session.md

Co-authored-by: Matt Henkes <[email protected]>

* Update content/api/cypress-api/session.md

Co-authored-by: Matt Henkes <[email protected]>

* fix markdown

* Update content/api/cypress-api/session.md

* Apply suggestions from code review

Co-authored-by: Matt Henkes <[email protected]>

* V12 ChangeLog (#4896)

Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Blue F <[email protected]>
Co-authored-by: DEBRIS APRON <[email protected]>
Co-authored-by: Ben M <[email protected]>
Co-authored-by: Ryan Manuel <[email protected]>
Co-authored-by: Chris Breiding <[email protected]>

Co-authored-by: DEBRIS APRON <[email protected]>
Co-authored-by: Blue F <[email protected]>
Co-authored-by: Rachel <[email protected]>
Co-authored-by: Zach Bloomquist <[email protected]>
Co-authored-by: Matt Schile <[email protected]>
Co-authored-by: Matt Henkes <[email protected]>
Co-authored-by: Chris Breiding <[email protected]>
Co-authored-by: Bill Glesias <[email protected]>
Co-authored-by: Ben M <[email protected]>
Co-authored-by: Ryan Manuel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants