feat: _addQuery()

[BlueWinds]

• Closes Detached DOM: Commands._addQuery() #23550

User facing changelog

This PR contains three user-facing changes:

• The addition of Cypress.Commands.addSelector(), in packages/driver/src/cypress/commands.ts and packages/driver/src/cypress/command_queue.ts. This currently an internal API - it will be added to the documentation and finalized as part of upcoming work.
• Migrating .get() to be the first selector.
• Moving Aliases to use concept of "subject chains" rather than re-running commands via the command queue, in packages/driver/src/cy/commands/querying/querying.ts and packages/driver/src/cy/commands/aliasing.ts.

Additional details

TODO

Steps to test

TODO

How has the user experience changed?

.get() should no longer be susceptible to Detached DOM errors. I intend to write additional tests around it, but please note the one I had to delete in within.cy.ts - it no longer fails, because addSelector() isn't susceptbile to that issue!

PR Tasks

• Have tests been added/updated? Some have been added, but I would like to add more before merging, especially around custom commands and selectors.
• Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [n/a] Has a PR for user-facing changes been opened in cypress-documentation? API is not public facing, and one query alone has no behavioral changes (they only start occurring when queries interact with each other)
• [n/a] Have API changes been updated in the type definitions?

[cypress-bot]

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.

[cypress]

---

Test summary

41100 • 0 • 3396 • 0 • 4

---

Run details

Project	cypress
Status	Passed
Commit	9263b69
Started	Sep 28, 2022 3:39 PM
Ended	Sep 28, 2022 3:55 PM
Duration	16:55 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	runs.cy.ts		1
1	App: Runs > Runs - Create Project > when a project is created, injects new projectId into the config file, and sends expected UTM params
	specs_list_latest_runs.cy.ts		1
1	App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs
	e2e/origin/config_env.cy.ts		1
1	cy.origin- Cypress.config() > serializable > overwrites different values in secondary, even if the Cypress.config() value does not exist in the primary
	next.cy.ts		1
1	Working with next-12.1.6 > should show compilation errors on src changes

---

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

[BlueWinds]

Aliasing is no longer a 'special utility command', it exists in the command queue and runs the same as any other.

[BlueWinds]

Because the alias is now uses subject chains - which invoke the aliased commands directly - we've effectively made "using the correct jquery instance" the responsibility of the executing commands (which all already handle it properly).

[BlueWinds]

This series of tests asserts on the internal state of Cypress - I've changed the shape of how aliases are stored, so the tests needed updating. This is not a "user-facing" test update.

[BlueWinds]

Aliases no longer run through the command queue, so this set of tests doesn't make much sense anymore. They instead use subject chains to accomplish the same goal.

"A subject chain" is essentially the minimal set of information needed to "replay commands that led of to a current subject" - except now it's used everywhere in Cypress, so all commands can benefit from it, rather than as a special case in aliasing.

[BlueWinds]

This is an actual behavioral change: We only set the log message to child when using the actual subject, not when asserting on something which happens to strictly equal the subject.

I don't see that this has any practical relevance for users. It's a very obscure situation, and is display-only (no tests that previously passed will now fail, nor vis-versa).

[flotwig]

log:added is in our public API in case anyone reading this was wondering: https://docs.cypress.io/api/events/catalog-of-events#Cypress-Events But I agree this is extremely unlikely to cause breakage.

[BlueWinds]

The tests added as part of the linked issue are intact, and pass without changes. This logic here is no longer necessary due to cleaner handling of how subjects chain between commands.

[BlueWinds]

Because queries return a function, it makes it super easy to isolate all the alias-related code in cy.get from the DOM-related code.

 if (aliasRe.test(selector)) {
      return getAlias.call(this, selector, log, cy)
    }

So .get() just has an early return (this function) if the arguments look like an alias.

[BlueWinds]

In addition to the complex case of .get(), I wanted to include a simple query in this PR, to show the easier case.

.root() is as simple as it gets. This new version is more powerful than the old one - it retries if the root is detached from the DOM! This would be fairly unusual in the case of .root(), but I wanted to highlight how short a query can be (and how easy for the community to write properly) while still having a great deal of builtin retryability / other things that make Cypress commands great.

[lmiller1990]

Good candidate for a post 11.x blog post

[BlueWinds]

We already have tests around the user-facing behavior - this test asserting on the internal state that leads to this behavior is unnecessary.

[emilyrohrbough]

@BlueWinds Can you link that test?

[BlueWinds]

The rest of this file is asserting on the output of the alias (eg, that we can store and retrieve aliases). It's not a specific test, but the whole file. 🤷‍♀️

[BlueWinds]

This tests currently looks at cy.commandFns, which selectors are not added to.

TODO: Look into this test more, and see what it's actually doing and asserting on. Will do this as part of the next push, related to selectors-invoking-selectors.

[flotwig]

Interesting idea. How would this work? My first thought is use a MutationObserver to prevent duplicating snapshots when nothing has changed in the DOM?

[BlueWinds]

I was thinking about doing it a little bit less intelligently, and just noting if we're taking multiple snapshots synchronously - in a couple of places we do command.get('logs').forEach() sorts of things, where we iterate through commands and / or logs and snapshot all of them.

If we're looping like that without a chance for external JS code to execute, then we know the snapshots can be shared. Mutation observer would be a more intelligent way to do it - just keep a pointer to the most recent snapshot, and invalidate it whenever the DOM mutates.

Dunno, just a thing I was thinking about as I looked at this code. It's come up a couple of times (with Emily looking at memory usage, and also Brian brought up the idea at one point).

[lmiller1990]

Also interested in solving the "snapshots are big and memory hungry" problem. Another dumb way (I guess someone thought of this and dismissed it for reasons I hope I'm about to learn) is just to serialize the snapshots and compare them before/after, eg using XMLSerializer.

As an aside, when we talk about memory usage, is the issue primarily browser memory usage (this is my understanding). If so, the other solution I've seen floated (also talked about) would just be saving them in some database (sqlite?) on the server and shuttling them back/forth as needed via a websocket.

The obvious benefit of this is less complexity - just consumes a lot more data. Potential positive would be you can keep the snapshots for use later on (could be useful for many cool things, like replaying something that happened on CI in the Cloud or locally...).

[BlueWinds]

I'd prefer to move in the opposite direction, to shuttle less over the air between app <-> server, rather than more.

Serializing is expensive, even if we don't retain it in memory. Repeatedly snapshotting and throwing away the snapshots if they're identical is a great way to keep CPU manufacturers in business. :P

[brian-mann]

You'd track the diffs using MutationObserver and only serialize the patches that describe the minimum set of changes, and then rebuild them on demand to a particular state / time

[lmiller1990]

Some early comments, I will play with this for a bit and probably make some more, I'm still internalizing things.

[lmiller1990]

Do we know the full type definition for subjectChain? Getting this a bit more type-safe would be really useful - right now without a lot of context on how it all works, it's pretty difficult for new people to dive into driver, because you need this mental model of what exactly subject and subjectChain are. I'm finding I need to do a lot of logging and poking around to figure out what I'm looking at.

Not sure if it's practical to add them, but even something simple would be useful.

[BlueWinds]

Added some types to the PR. SubjectChain looks like so:

type QueryFunction = (any) => any
export type SubjectChain = [any, ...QueryFunction[]];

any in this context is a complete description - any value is valid for the first entry, including undefined, and QueryFunctions can take anything (one argument) and return anything (one argument).

Commands.addQuery (name: string, fn: () => QueryFunction)

Also added these types: a query is a function that returns a QueryFunction (department of redundancy department is redundant :D ).

[lmiller1990]

I think it's still useful to know that something returns itself, even if it feels a bit redundant. Eventually it would be nice to support something like

cy.get<HTMLButtonElement>('#button'). [only show relevant methods to HTMLButtonElement here]

Gotta start somewhere.

[BlueWinds]

Individual commands can potentially be more strictly typed - but I think you'll quickly find that most Cypress commands are so versatile that their types end up horribly messy. .get(), for example, can return an HTMLElement... or the result of an alias, which can be literally anything. '.first(), which looks like a method for traversing HTML collections, can also accept any[], or JQuery` as a previous subject. 🤷‍♀️

[lmiller1990]

Also interested in solving the "snapshots are big and memory hungry" problem. Another dumb way (I guess someone thought of this and dismissed it for reasons I hope I'm about to learn) is just to serialize the snapshots and compare them before/after, eg using XMLSerializer.

As an aside, when we talk about memory usage, is the issue primarily browser memory usage (this is my understanding). If so, the other solution I've seen floated (also talked about) would just be saving them in some database (sqlite?) on the server and shuttling them back/forth as needed via a websocket.

The obvious benefit of this is less complexity - just consumes a lot more data. Potential positive would be you can keep the snapshots for use later on (could be useful for many cool things, like replaying something that happened on CI in the Cloud or locally...).

[lmiller1990]

I left some more questions/comments 👍

[lmiller1990]

Huh, did not know about cy.now().

I do not fully understand this - I get a lot of things rely on the original cy.get() (makes sense) but if we overwrite it immediately, won't all those commands get the newer cy.get() which uses the new selector implementation? I'm probably missing something.

[BlueWinds]

The key is that that Commands.add() registers the function in cy.commandFns[name], while addQuery doesn't. So we register it as a command, creating cy.get() and cy.commandFns.get, then register it as a query, overriding cy.get().

https://github.com/cypress-io/cypress/pull/23791/files#diff-231d74866a51e1411d172ab6ca0658f7fa712e9676f216ba797c544c47dcbf94 contains the actual fix - registering queries separately (without a wrapping promise) and then updates our other commands to use cy.now('get') properly when .get is a query - but I didn't have that ready when I made this PR, and I think it makes sense to merge in separately regardless (since it's a breaking change, needs to go in 11.x branch and not develop).

[lmiller1990]

I was trying to find out where this was used so I pushed a commit throwing an error here: https://github.com/cypress-io/cypress/compare/issue-7306-addSelector-redux...lmiller1990/issue-7306-addSelector-redux?expand=1

Surprisingly only tests related to type() broke - is this what you'd expect? Why would this be case? I was expecting a lot more to fail. Does this mean only type() uses the old get implementation? That's a lot less than I was expecting.

CI: https://app.circleci.com/pipelines/github/cypress-io/cypress?branch=lmiller1990%2Fissue-7306-addSelector-redux

[BlueWinds]

It's because the type() tests invoke cy.getAll(), which a custom command registered in support/utils.ts and uses cy.now('get', '@stuff') - which is the only way to hit this block of code.

Normal users use cy.get('@foo') (using the selector), and most of our commands use cy.get('button') (which isn't an alias and therefore misses this code block about aliases). Only the combination of aliasing and .now() can end up here, and .type() tests seem to be the only place in our code base that does so. 🤷‍♀️

[lmiller1990]

Hm ok - we will eventually refactor type to not go here, then we can delete this block (only used for one thing, which is type, which is hard to know by looking at this)?

[BlueWinds]

Yeah. This entire block - the whole add('get') section - will be removed in my next PR, which I'm currently working on and should have ready today or tomorrow. This is just a messy temporary state because I wanted to limit the scope of things going in at once.

[emilyrohrbough]

Why is the first subject chain undefined? Shouldn't the only subject chain be .get()? Shouldn't we assert the second subject chain is actually .get()?

[BlueWinds]

Subject chains always start with a "primitive" value, and then append any number of functions that then chain off this value.

Consider these two cases:

cy.get('button').contains('Login')

cy.get('button').click().contains('Login')

In the first case, we make a subject chain like [undefined, getFn(), containsFn()]. In the second one, we start with that same value, but .click() replaces the subject chain rather than adds to it - so we end up with [<button>, containsFn()].

I start subject chains with 'undefined' so that they always have a consistent shape: a primitive value, followed by 0 or more functions that are applied to it.

[BlueWinds]

For the second part - yes, yes we should assert that the second entry is .get(). Added an assertion.

[BlueWinds]

Discussing chainer and command IDs with @emilyrohrbough in slack, we came to the conclusion that it would be very nice for debugging if it were clear which commands belong to which chainers; thus this change (and the associated one above in packages/driver/src/cypress/chainer.ts).

This results in command and chainer IDs that look like this, where it's easy to tell at a glance what belongs to what. It also eliminates the confusion that chainerIds weren't sequential - _.uniqueId() shares only a single counter, so you'd get chainer-2, cmd-3, chainer-4, etc.

This is only to make IDs easier to reason about and debug, no user-visible impact.

[BlueWinds]

I've pushed some additional changes to this branch since the last review, merging in develop and fixing some subsequent merge conflicts and test failures.

Re-requesting review from those who've approved, and thank you for your patience while I got this into shape again!

[lmiller1990]

Will be interested to review the documentation PR associated with queries (we will need to make sure how they work and requirements/limitations are clear to users, so they can author their own).

[BlueWinds]

After discussing this with Brian, we're holding off making the API public, which is why the PR switched to _addQuery, indicating an internal method. Documentation will wait until the end of the project, when we're ready to remove the underscore and have worked out all the kinks.

[lmiller1990]

Sorry I should clarify; I'll be interested in seeing it when it's available (not saying we should have it done now - definitely can come later).

[lmiller1990]

I ran the tests and poked around, getting a better understanding of all this. I'll approve since the code works and we've been over it a few times, but I do have one last question. It's more for my understanding.

[lmiller1990]

I am not fully understanding what's going on here - why does _addQuery need to grab the userInvocationStack? Isn't this for error handling? Do we expect that here when they initially call useQuery (or is this to handle the cases where they used useQuery incorrectly?

[BlueWinds]

Good question, it took me a while to figure out how stack traces work in Cypress.

This function is literally cy.get(): A few lines above, we have cy[name] = function (...args) { ...this function... }, so the stack trace here is the transition point between 'user test code' and 'internal Cypress code'. We capture it so that later, when the command actually executes and possibly throws an error, we can point out to users where in their code an error occurred.

Here are a couple of slightly simplified stack traces to show why this is the best place. With the simple command cy.get('body'), the stack trace at this point looks like:

    name                       cy.ts:797  <-- This anonymous function, which is literally `cy.get`
    js                         querying.cy.js:16   <-- The code in the test file calling cy.get('body')
    __stackReplacementMarker   cy.ts:85
    ...lots of internal Cy stuff...

Here's what a stack trace looks like when an error is thrown inside cy.get():

Any error in the command is thrown much later, as part of command queue execution - the location where cy.get() was called in the user's code is long lost, and this stack trace contains no useful information for the user trying to figure out how they wrote their test wrong / how their application is failing.

Thus, for queries and commands both, we capture the user stack trace when the command is added to the queue, so that we can display this if the command throws an error later while executing.

(none of this is query specific; commands work the same way with regards to userStackTraces)

[lmiller1990]

Ok, that explains how this works (had a vague idea, but this clarified). Last question would be why was this added here specifically in this PR? Did we move the logic here from somewhere else? Or did the useQuery addition require this to be added here?

[BlueWinds]

It's added here because _addQuery is a mirror to the existing addCommand - you can see the exact same line,

const userInvocationStack = $stackUtils.captureUserInvocationStack(cy.specWindow.Error)

up earlier in this file, line 791 inside addCommand.

_addQuery and addCommand are fairly similar in a lot of ways, but the details differ enough that I didn't want to mesh in a bunch of if (isQuery) {...} code branches into addCommand, and decided that some code duplication was the lesser of two evils.

[BlueWinds]

Currently failing tests are flake; cypress-schematic will be fixed in develop soon (thanks Ryan!), and the driver-integration-tests-electron-experimentalSessionAndOrigin is known flake from cy.origin + cy.go().