-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Refactor cy.state('subject') and cy.then()
#22742
Conversation
Thanks for taking the time to open a PR!
|
@@ -27,6 +27,7 @@ export default (Cypress, userOptions: Cypress.LogGroup.Config, fn: Cypress.LogGr | |||
const endLogGroupCmd = $Command.create({ | |||
name: 'end-logGroup', | |||
injected: true, | |||
args: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this change for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -72,7 +72,7 @@ const convertObjectToSerializableLiteral = (obj): typeof obj => { | |||
}) | |||
|
|||
currentObjectRef = Object.getPrototypeOf(currentObjectRef) | |||
} while (currentObjectRef) | |||
} while (currentObjectRef && currentObjectRef !== Object.prototype && currentObjectRef !== Date.prototype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So current plan:
- Add a shim on
cy.state('subject')
in this PR so it keeps working as achore
. 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I've updated the PR description to note the change and the rational behind it.
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
this.state('subjectLinks', links) | ||
} | ||
|
||
breakSubjectLinksToCurrentChainer () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I started writing and then words came out. Many lines of comments added around the new cy functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are amazing! 🤩
…removal (and be more complete)
@@ -13,75 +13,6 @@ const builtInCommands = [ | |||
addNetstubbingCommand, | |||
] | |||
|
|||
const reservedCommandNames = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 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.
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.
|
@@ -27,6 +27,7 @@ export default (Cypress, userOptions: Cypress.LogGroup.Config, fn: Cypress.LogGr | |||
const endLogGroupCmd = $Command.create({ | |||
name: 'end-logGroup', | |||
injected: true, | |||
args: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is an injected command, why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
* 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', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this warning show in the reporter as a log or in the devTools console?
Is this observed in run mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the devtools console.
It does not show up at all in run mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Nice work!
linkSubject (fromChainerId, toChainerId) { | ||
const links = this.state('subjectLinks') || {} | ||
|
||
links[fromChainerId] = toChainerId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is fromChainerId
the child / chained command? ex:
cy.get().should().should() or c1 -> c2 -> c3 would map to:
{
'c2': 'c1',
'c3': 'c1,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly. I will add a note to change this to child / parent in my next PR, that will be clearer variable names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh thats right. 👍🏻 thanks!
* 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>
User facing changelog
cy.state('subject')
is deprecated, and reading from it will log a warning to the console. Prefercy.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 oncy.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
cypress-documentation
?type definitions
?