chore: Reapply state refactor #23092
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run details
View run in 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 |
| @@ -27,7 +27,7 @@ mainBuildFilters: &mainBuildFilters | |||
| branches: | |||
| only: | |||
| - develop | |||
| - revert-22742 | |||
| - reapply-state-refactor | |||
There was a problem hiding this comment.
I've updated this branch to run the complete set of linux tasks, including the test-binary-against-todomvc-firefox stage which caused to be be reverted previously.
| @@ -29,6 +29,11 @@ describe('src/cy/commands/aliasing', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| it('stores the lookup as an alias when .then() is an intermediate', () => { | |||
There was a problem hiding this comment.
This was the minimal repro of the issue causing the todomvc failure, included in the monorepo to prevent any future regression. Explanation about what was going wrong and how it was fixed in another comment.
| @@ -860,8 +846,15 @@ export class $Cy extends EventEmitter2 implements ITimeouts, IStability, IAssert | |||
| return memo | |||
| }, [initialCommand]) | |||
|
|
|||
| const chainerId = this.state('chainerId') | |||
There was a problem hiding this comment.
Here is the change which fixes the todomvc regression. The source of the error was very, very distant from the error that was ultimately thrown.
In the old code, replayCommandFrom() updates the chainerId before cloning - it updates the original command. This was causing a chain of unintended side-effects, ultimately resulting in an undefined subject, rather than the one properly stored in cy.state('subjects').
In the new version, it clones and then updates, leaving the original command intact. Cypress no longer gets confused when replaying commands.
I'm not terribly fussed about the exact details here - this entire "replayCommandsFrom" logic is going to be removed as part of the Detached DOM work. As long as it keeps working in the interim (and we have a lot of tests ensuring it does), I'm not much fussed about exactly what it looks like.
| * As part of the Detached DOM effort, we're changing the way subjects are determined in Cypress. | ||
| * While we usually consider cy.state() to be internal, in the case of cy.state('subject'), | ||
| * cypress-testing-library, one of our most popular plugins, relies on it. | ||
| * https://github.com/testing-library/cypress-testing-library/blob/1af9f2f28b2ca62936da8a8acca81fc87e2192f7/src/utils.js#L9 | ||
| * | ||
| * 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). |
There was a problem hiding this comment.
Nice work finding usage and shiming to support cypress-testing-library
Co-authored-by: Matt Henkes <[email protected]>
|
Great! Will leave the CI dance and merging to you. |
User facing changelog
cy.state('subject')is deprecated, and reading from it will log a warning to the console. Prefercy.currentSubject()instead.Additional details
These refactors were previously merged, and reverted when it became clear it was causing issues with one of our develop-only tests. #23047. This PR contains a fix for that test failure - see comments below for 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-libraryrelies 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/drivere2e 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?