chore: Refactor cy.state('subject') and `cy.then()` #22742

BlueWinds · 2022-07-11T21:31:54Z

User facing changelog

cy.state('subject') is deprecated, and reading from it will log a warning to the console. Prefer cy.currentSubject() instead.

Additional details

This PR is a follow up to #22571, and refactors how Cypress keeps track of the current subject. No behavior has changed, but this is in preparation for addressing Detached DOM errors (#7306).

While we would normally consider internal state to be internal, cypress-testing-library relies on cy.state('subject'). Therefore, this PR includes a shim to keep that call working as before. Once this PR is released, we will follow up with them to move over to the new call, ensuring that no end users (with or without cypress-testing-library) ever experience disruption (beyond the console warning).

Steps to test

The automated tests cover this code extensively, especially the packages/driver e2e tests.

How has the user experience changed?

No change. There is less indirection around how commands are added and invoked, which results in less memory churn and CPU usage (but only a ms here or there, not really noticeable).

PR Tasks

Have tests been added/updated? Yes
[n/a] 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?
[n/a] Have API changes been updated in the type definitions?

…ctor

cypress-bot · 2022-07-11T21:31:57Z

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.

flotwig · 2022-07-12T16:35:28Z

packages/driver/src/cy/logGroup.ts

@@ -27,6 +27,7 @@ export default (Cypress, userOptions: Cypress.LogGroup.Config, fn: Cypress.LogGr
  const endLogGroupCmd = $Command.create({
    name: 'end-logGroup',
    injected: true,
+    args: [],


what's this change for?

Consistency. This homogenizes $Commands created here with commands created everywhere else in Cypress. Adding the empty array here means that elsewhere in the PR I can assume that all commands have an args array.

I've honestly forgotten exactly where it was throwing an error, but it was somewhere pretty arcane, and this one-line change meant I didn't have to add existence checks deep inside the command-queue.

flotwig · 2022-07-12T16:48:44Z

packages/driver/src/util/serialization/index.ts

@@ -72,7 +72,7 @@ const convertObjectToSerializableLiteral = (obj): typeof obj => {
    })

    currentObjectRef = Object.getPrototypeOf(currentObjectRef)
-  } while (currentObjectRef)
+  } while (currentObjectRef && currentObjectRef !== Object.prototype && currentObjectRef !== Date.prototype)


what's the intent of this change?

side note and not expected from this PR: while loops give me the creeps after dealing with a few bugs where errors in the case statement would lock up the browser, it would be nice if this was written as a recursive function so it could eventually reach a max stack size exception

This is a performance optimization I noticed while debugging. It's otherwise unrelated to this PR.

If you turn on "break on exceptions" and add "break on caught exceptions" in the chromium debugger, you can see that we hit this code literally thousands of times for every test, because for every object we try to serialize, we recurse down to Object.prototype and iterate over its properties, throwing and catching dozens of "can't serialize native code" exceptions.

Same with dates - we see them as objects, recurse down into the prototype, and then throw and catch dozens of exceptions.

flotwig · 2022-07-12T16:50:04Z

packages/driver/src/cypress/command_queue.ts

@@ -247,7 +249,7 @@ export class CommandQueue extends Queue<$Command> {
      // we're finished with the current command so set it back to null
      this.state('current', null)

-      this.state('subject', subject)


This PR removes cy.state('subject'), right? I think we should be careful here - it's not a publicly-documented API, but it seems like it's used in a couple of Cypress plugins, testing-library/cypress-testing-library and Lakitna/cypress-commands (tests only):

https://github.com/search?q=cy.state%28%27subject%27%29&type=code

What do you think about still setting cy.state('subject') just for backwards-compatibility, and opening a second PR for 11.0.0 that will remove it entirely, so the plugins have some time to update? cypress-testing-library gets 3MM npm downloads a month, so I'm worried about breaking those users.

This PR changes the formatting of it, to be an object rather than a single entry, in preparation for changing it even further down the line (into an even more complex data structure). You're right though, if testing-library is relying on it, then this is a breaking change and I absolutely cannot make it as part of a chore.

Hm. This is really rough though. The call cy.state('subject') is exactly what I'm trying to get rid of - it's the source of detached DOM errors, distilled to its essence. Let me think on this a bit and figure out what to do here.

Ok, so thinking about this further, I think we can in fact leave cy.state('subject') 'mostly working', and add a deprecation warning to it. The basic problem is that the it does not map cleanly to the current subject, and never has. I guarantee you every third-party project using cy.state('subject') has subtle bugs around the use of cy.within and cy.as.

That's what I mean when I say "mostly working" - I can add a shim to keep it working as-is (only a little bit broken).

So current plan:

Add a shim on cy.state('subject') in this PR so it keeps working as a chore. Merge this PR.

In next PR, introduce selectors / function chains as subject, as a feat. Keep the shim working.

Open a PR with cypress-testing-library to move them to the new functionality.

Remove shim in breaking 11.x release eventually.

Updated in latest commit. I now store the subject mapping in cy.state('subjects'), with cy.state('subject') being an alias (that logs a deprecation warning) for cy.currentSubject().

Nice catch here. We should probably make a PR against testing-library once this lands, to save them the time/effort, and while this is fresh in our minds.

Does PR likely needs to be flagged for release so a note can be made in the changelog that this usage has been deprecated if we are porting this functionality?

Good call. I've updated the PR description to note the change and the rational behind it.

…ctor

emilyrohrbough · 2022-07-15T17:51:42Z

packages/driver/src/cy/commands/querying/within.ts

@@ -76,6 +78,10 @@ export default (Commands, Cypress, cy, state) => {
      })
    }

+    // TODO: Rework cy.within to use chainer-based subject chaining, rather than its custom withinSubject state.
+    // For now, we leave this logic in place and just ensure that the new rules don't interfere with it.
+    cy.breakSubjectLinksToCurrentChainer()


Could you explain this? If I am understanding correctly, it seem like this would break the following?

cy.get('.form').within(() => { do something}).should('contain', 'submit')

So the key is that cy.within works differently from cy.then - it never changes the subject. According to our docs (https://docs.cypress.io/api/commands/within#Yields):

.within() yields the same subject it was given from the previous command.
Trying to return a different element the .within callback have no effect:

cy.get('#within-yields') .within(() => { // we are trying to return something // from the .within callback, // but it won't have any effect return cy.contains('Child element').should('have.class', 'some-child') }) .should('have.id', 'within-yields')

So cy.within always matches the two conditions I mentioned in my other comment - the callback contains cypress commands, and we do not want to use their resolved subject as the new subject of the parent chainer. The existing automated tests back up this behavior.

I didn't know this about cy.within - I learned it by looking at the tests, being confused, then looking at the docs to see if the tests were correct. 🤣 I can't speak to why it works this way, just that it does and I'm trying to retain all the existing rules.

Yeah, .within() is a bit interesting in that regard -- but curious what these new rules are exactly? It seems this logic is maintaining parity with the current logic?

Does your TODO comment mean you'd like to change this behavior later on or that you need to remove cy.breakSubjectLinksToCurrentChainer() ?

Yes, this logic is maintaining parity with existing functionality. The 'two rules' I mentioned are just guidelines to help understand when breakSubjectLinksToCurrentChainer is appropriate to use.

The TODO comment is that I'll need to refactor cy.within as part of the effort of implementing selectors / fixing detached DOM. The goal will still be to maintain the existing functionality as part of any further work in here.

emilyrohrbough · 2022-07-15T17:54:43Z

packages/driver/src/cypress/cy.ts

+    this.state('subjectLinks', links)
+  }
+
+  breakSubjectLinksToCurrentChainer () {


Thoughts on adding in some docs on what these are doing? Seems like a lot of code to replace .state('subject')

Curious on what rules/scenarios are breakSubjectLinksToCurrentChainer for should be used?

breakSubjectLinksToCurrentChainer is used when the following are true:

A command callback contains cypress commands.

You do not want to use the subject of those commands as the new subject of the parent.

Whenever a command callback executes cypress commands, we automatically create a link between those commands and the parent, so that the following example works:

cy.then(() => { return cy.get('body') }).log()

In this case, we want to log the body. But remember the order of execution here:

cy.then()

.log(). We haven't executed the callback yet - we're still queuing up commands from the test function.

Now we begin executing the command queue. .then() calls () => { ... }

cy.get() is invoked as part of the then() callback.

So when .get() executes, we link it to the 'outside' chainer. Condition 1 matches - the callback contained cypress commands - but not condition 2. We want to use the subject of these commands as the new subject of cy.then().

But consider this example instead:

cy.then(() => { cy.get('body') return 'foo' }).log()

Here we want to log 'foo'. But we don't know that when cy.get() is added to the queue - we don't yet know the return value of the .then() callback! The execution order looks like this:

cy.then()

.log()

Now we begin executing the command queue. .then() calls () => { ... }

cy.get() is invoked as part of the then() callback. We link this inner chainer's eventual subject - which we don't know yet, we haven't resolved .get(), only put it in the command queue.

The then() callback returns 'foo'.

It's at this point that the .then() command realizes "Oh, I have a return value which is not a $Chainer. I want to use that instead of the results of any commands executed inside my callback." Condition 2 is satisfied - so it calls breakSubjectLinksToCurrentChainer().

Ok, so that novel of a comment aside, I'm unsure if I want to document this right now - I believe the next step (introducing the concept of 'selectors' and subject chains) will change how this works to some extent, and commenting the intermediate stage extensively didn't feel quite right.

With that said, I'm not exactly sure how much will change - it may be worth documenting the current state in-code, rather than just in my head or in PR comments like this. I'll add a comment here (but probably not a complete description of the system).

Nevermind, I started writing and then words came out. Many lines of comments added around the new cy functions.

These comments are amazing! 🤩

…ect')

…removal (and be more complete)

BlueWinds · 2022-07-18T19:42:39Z

packages/driver/src/cypress/commands.ts

@@ -13,75 +13,6 @@ const builtInCommands = [
  addNetstubbingCommand,
 ]

-const reservedCommandNames = {


I remember when we added this list a few months ago - it's already out of date and incomplete, even before the changes in this PR.

I've moved it to use Object.keys(cy) (before we add any commands), which adds things like $$ and _removeListener (inherited on $Cy from EventEmitter) to the reserved list.

I'm fairly certain users weren't adding custom commands with these names - doing so would have completely broken Cypress, so I don't consider adding them here (strengthening our validation) to be a breaking change.

cypress · 2022-07-18T20:01:15Z

Test summary

37725 • 0 • 456 • 0 • 11

Run details


Project	cypress
Status	Passed
Commit	`ec01d6d`
Started	Jul 18, 2022 7:44 PM
Ended	Jul 18, 2022 8:01 PM
Duration	16:27 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/actions/click.cy.js	1
1	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
	cypress/proxy-logging.cy.ts	4
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged first
2	... > intercept log has consoleProps with intercept info
3	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged first
4	... > intercept log has consoleProps with intercept info
This comment includes only the first 5 flaky tests. See all 11 flaky tests in the 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

BlueWinds · 2022-07-18T20:12:50Z

This PR is slightly larger than I'd like, apologies for lumping a couple of changes into the same batch. I'll try and start a separate branch for any further (non-review-related) changes.

lmiller1990

I never realized how complex the driver and the "subject" concept was until I started following these PRs. I also don't see many references to currentSubject() in our tests - should there be more of those, if this is becoming part of the public API (or is it remaining private, like cy.state('subject')?

I'm still internalizing how this all works, and don't feel like I've gone into enough depth for an approval to have much weight, but good to see incremental, careful improvements to this core bit of our API.

BlueWinds · 2022-07-20T20:04:52Z

I never realized how complex the driver and the "subject" concept was until I started following these PRs. I also don't see many references to currentSubject() in our tests - should there be more of those, if this is becoming part of the public API (or is it remaining private, like cy.state('subject')?

It's remaining "mostly private". You only need to read it in exceptional cases. I'd say it should be entirely private, but since the community is reading it sometimes, we need to continue to support them.

cypress-testing-library is reading the subject directly because they want to integrate with our .within() command - they mostly care about cy.state('withinSubject'), rather than cy.state('subject'). I want to look at our implementation of .within to see if I can make their use easier, but that's something to mess with at a later date.

emilyrohrbough · 2022-07-20T20:13:27Z

packages/driver/src/cy/logGroup.ts

@@ -27,6 +27,7 @@ export default (Cypress, userOptions: Cypress.LogGroup.Config, fn: Cypress.LogGr
  const endLogGroupCmd = $Command.create({
    name: 'end-logGroup',
    injected: true,
+    args: [],


Given this is an injected command, why do we need this?

Without this, we get errors in commands/querying/root.cy.js and commands/querying/within.cy.js, saying "args is not iterable." from this line in command_queue.ts:

args = [command.get('chainerId'), ...args]

I could add a guard against commands that don't have args in there, or I could ensure that every command has an args array and not need to guard against it. I prefer consistent object shapes over null checks when possible.

The follow-up PR I'm currently working on removes the concept of 'injected' entirely (while maintaining all existing functionality and without editing any of the related tests).

emilyrohrbough · 2022-07-20T20:16:21Z

packages/driver/src/cypress.ts

+     * Therefore, we've added this shim to continue to support them. The library is actively maintained, so this
+     * shouldn't need to stick around too long (written 07/22).
+     */
+    Object.defineProperty(this.state(), 'subject', {


Does this warning show in the reporter as a log or in the devTools console?

Is this observed in run mode?

In the devtools console.

It does not show up at all in run mode.

ryanthemanuel

Looking good. Nice work!

emilyrohrbough · 2022-07-20T20:31:08Z

packages/driver/src/cypress/cy.ts

+  linkSubject (fromChainerId, toChainerId) {
+    const links = this.state('subjectLinks') || {}
+
+    links[fromChainerId] = toChainerId


is fromChainerId the child / chained command? ex:
cy.get().should().should() or c1 -> c2 -> c3 would map to:

{ 'c2': 'c1', 'c3': 'c1, }

~~Yep, exactly. I will add a note to change this to child / parent in my next PR, that will be clearer variable names.~~

Ok, reading your comment more carefully, that's not quite right.

$Chainers are created when you invoke commands directly off Cypress. So your example would never create c2 / c3. You'd have to do something like

cy.get() // Creates c1 .find() // Still part of c1 .then(() => { // .then is also part of c1 cy.window() // Creates c2 cy.window() // Creates c3 }) .then(() => { // This .then is part of c1 cy.window() // Creates c4 })

I'll still rename these arguments to childChainerId / parentChainerId, since that does make sense.

ahhh thats right. 👍🏻 thanks!

This reverts commit 0ed8dd5.

* Revert "chore: Refactor cy.state('subject') and `cy.then()` (#22742)" This reverts commit 0ed8dd5. * Run more tests

This reverts commit 51ef99a.

* Revert "chore: reverting #22742 (#23047)" This reverts commit 51ef99a. * Fix for aliases when .then() is in the chain * Run all tests on branch * Fix silly mistake * Fix broken test (again) * Update packages/driver/cypress/e2e/cypress/cy.cy.js Co-authored-by: Matt Henkes <mjhenkes@gmail.com> Co-authored-by: Matt Henkes <mjhenkes@gmail.com>

BlueWinds added 9 commits June 28, 2022 11:48

Refactor chainer / Commands.add for readability

a3a9e35

Fix invoking wrong function, add comment

d3aef86

WIP, still failing tests

5d81298

Fix additional driver tests

74fa0e5

Fix subject for utility commands

916e6f1

Better fix for utility functions

7021ba8

Merge remote-tracking branch 'origin/develop' into issue-7306-addSele…

834ccae

…ctor

Fix more tests

ac86823

Another pass of test fixes

485a3fa

BlueWinds requested a review from a team as a code owner July 11, 2022 21:31

BlueWinds requested review from mschile, flotwig and ryanthemanuel and removed request for a team and mschile July 11, 2022 21:31

flotwig reviewed Jul 12, 2022

View reviewed changes

BlueWinds mentioned this pull request Jul 12, 2022

Re-query elements that are found 'detached' from the DOM #7306

Closed

Merge remote-tracking branch 'origin/develop' into issue-7306-addSele…

45689b6

…ctor

emilyrohrbough reviewed Jul 15, 2022

View reviewed changes

BlueWinds added 2 commits July 18, 2022 12:26

Additional comments, added shim to continue supporting cy.state('subj…

cf61e19

…ect')

Adjust reserved command names to no longer require manual addition / …

ec01d6d

…removal (and be more complete)

BlueWinds commented Jul 18, 2022

View reviewed changes

BlueWinds requested review from emilyrohrbough and flotwig July 18, 2022 19:42

flotwig approved these changes Jul 19, 2022

View reviewed changes

lmiller1990 reviewed Jul 20, 2022

View reviewed changes

emilyrohrbough reviewed Jul 20, 2022

View reviewed changes

ryanthemanuel reviewed Jul 20, 2022

View reviewed changes

emilyrohrbough reviewed Jul 20, 2022

View reviewed changes

ryanthemanuel approved these changes Jul 20, 2022

View reviewed changes

emilyrohrbough approved these changes Jul 20, 2022

View reviewed changes

BlueWinds merged commit 0ed8dd5 into develop Jul 20, 2022

BlueWinds deleted the issue-7306-addSelector branch July 20, 2022 21:30

mjhenkes added a commit that referenced this pull request Aug 1, 2022

Revert "chore: Refactor cy.state('subject') and cy.then() (#22742)"

08f0254

This reverts commit 0ed8dd5.

mjhenkes mentioned this pull request Aug 1, 2022

chore: reverting #22742 #23047

Merged

4 tasks

mjhenkes added a commit that referenced this pull request Aug 1, 2022

chore: reverting #22742 (#23047)

51ef99a

* Revert "chore: Refactor cy.state('subject') and `cy.then()` (#22742)" This reverts commit 0ed8dd5. * Run more tests

BlueWinds added a commit that referenced this pull request Aug 2, 2022

Revert "chore: reverting #22742 (#23047)"

1f07e9f

This reverts commit 51ef99a.

chore: Refactor cy.state('subject') and cy.then() #22742

chore: Refactor cy.state('subject') and cy.then() #22742

Conversation

BlueWinds commented Jul 11, 2022 • edited

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlueWinds Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Jul 18, 2022

Test summary

Run details

Flakiness

BlueWinds commented Jul 18, 2022

lmiller1990 left a comment • edited

Choose a reason for hiding this comment

BlueWinds commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanthemanuel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlueWinds Jul 20, 2022 • edited

Choose a reason for hiding this comment

BlueWinds Jul 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: Refactor cy.state('subject') and `cy.then()` #22742

chore: Refactor cy.state('subject') and `cy.then()` #22742

BlueWinds commented Jul 11, 2022 •

edited

BlueWinds Jul 18, 2022 •

edited

lmiller1990 left a comment •

edited

BlueWinds Jul 20, 2022 •

edited

BlueWinds Jul 20, 2022 •

edited