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

Blue F · web-flow · commit 0ed8dd57b61c · 2022-07-20T14:30:14.000-07:00
* Refactor chainer / Commands.add for readability

* Fix invoking wrong function, add comment

* WIP, still failing tests

* Fix additional driver tests

* Fix subject for utility commands

* Better fix for utility functions

* Fix more tests

* Another pass of test fixes

* Additional comments, added shim to continue supporting cy.state('subject')

* Adjust reserved command names to no longer require manual addition / removal (and be more complete)
diff --git a/packages/driver/cypress/e2e/commands/assertions.cy.js b/packages/driver/cypress/e2e/commands/assertions.cy.js
@@ -806,7 +806,7 @@ describe('src/cy/commands/assertions', () => {
       })
 
       cy.get('body').then(() => {
-        expect(cy.state('subject')).to.match('body')
+        expect(cy.currentSubject()).to.match('body')
       })
     })
 
diff --git a/packages/driver/cypress/e2e/commands/commands.cy.js b/packages/driver/cypress/e2e/commands/commands.cy.js
@@ -58,7 +58,8 @@ describe('src/cy/commands/commands', () => {
       cy
       .get('input:first')
       .parent()
-      .command('login', 'brian@foo.com').then(($input) => {
+      .command('login', 'brian@foo.com')
+      .then(($input) => {
         expect($input.get(0)).to.eq(input.get(0))
       })
     })
diff --git a/packages/driver/cypress/e2e/commands/connectors.cy.js b/packages/driver/cypress/e2e/commands/connectors.cy.js
@@ -158,15 +158,16 @@ describe('src/cy/commands/connectors', () => {
         })
       })
 
-      it('should pass the eventual resolved thenable value downstream', () => {
+      it('should pass the eventually resolved thenable value downstream', () => {
         cy
         .wrap({ foo: 'bar' })
         .then((obj) => {
           return cy
           .wait(10)
           .then(() => {
             return obj.foo
-          }).then((value) => {
+          })
+          .then((value) => {
             expect(value).to.eq('bar')
 
             return value
@@ -309,7 +310,7 @@ describe('src/cy/commands/connectors', () => {
             return $div
           })
           .then(function () {
-            expect(cy.state('subject')).not.to.be.instanceof(this.remoteWindow.$)
+            expect(cy.currentSubject()).not.to.be.instanceof(this.remoteWindow.$)
           })
         })
       })
@@ -423,7 +424,6 @@ describe('src/cy/commands/connectors', () => {
 
           cy.get('div:first').invoke('parent').then(function ($parent) {
             expect($parent).to.be.instanceof(this.remoteWindow.$)
-            expect(cy.state('subject')).to.match(parent)
           })
         })
       })
diff --git a/packages/driver/cypress/e2e/cypress/cy.cy.js b/packages/driver/cypress/e2e/cypress/cy.cy.js
@@ -133,6 +133,16 @@ describe('driver/src/cypress/cy', () => {
         expect(userInvocationStack).to.include('.cy.js')
       })
     })
+
+    it('supports cy.state(\'subject\') for backwards compatability', () => {
+      cy.stub(Cypress.utils, 'warning')
+      const a = {}
+
+      cy.wrap(a).then(() => {
+        expect(cy.state('subject')).to.equal(a)
+        expect(Cypress.utils.warning).to.be.calledWith('`cy.state(\'subject\')` has been deprecated and will be removed in a future release. Consider migrating to `cy.currentSubject()` instead.')
+      })
+    })
   })
 
   context('custom commands', () => {
diff --git a/packages/driver/src/cross-origin/origin_fn.ts b/packages/driver/src/cross-origin/origin_fn.ts
@@ -105,7 +105,7 @@ export const handleOriginFn = (Cypress: Cypress.Cypress, cy: $Cy) => {
       queueFinished = true
       setRunnableStateToPassed()
       Cypress.specBridgeCommunicator.toPrimary('queue:finished', {
-        subject: cy.state('subject'),
+        subject: cy.currentSubject(),
       }, {
         syncGlobals: true,
       })
diff --git a/packages/driver/src/cy/commands/connectors.ts b/packages/driver/src/cy/commands/connectors.ts
@@ -100,47 +100,9 @@ export default function (Commands, Cypress, cy, state) {
 
     cy.once('command:enqueued', enqueuedCommand)
 
-    // this code helps juggle subjects forward
-    // the same way that promises work
-    const current = state('current')
-    const next = current.get('next')
-
-    // TODO: this code may no longer be necessary
-    // if the next command is chained to us then when it eventually
-    // runs we need to reset the subject to be the return value of the
-    // previous command so the subject is continuously juggled forward
-    if (next && next.get('chainerId') === current.get('chainerId')) {
-      const checkSubject = (newSubject, args) => {
-        if (state('current') !== next) {
-          return
-        }
-
-        // get whatever the previous commands return
-        // value is. this likely does not match the 'var current'
-        // command in the case of nested cy commands
-        const s = next.get('prev').get('subject')
-
-        // find the new subject and splice it out
-        // with our existing subject
-        const index = _.indexOf(args, newSubject)
-
-        if (index > -1) {
-          args.splice(index, 1, s)
-        }
-
-        return cy.removeListener('next:subject:prepared', checkSubject)
-      }
-
-      cy.on('next:subject:prepared', checkSubject)
-    }
-
     const getRet = () => {
       let ret = fn.apply(ctx, args)
 
-      if (cy.isCy(ret)) {
-        ret = undefined
-      }
-
       if (ret && invokedCyCommand && !ret.then) {
         $errUtils.throwErrByPath('then.callback_mixes_sync_and_async', {
           onFail: options._log,
@@ -161,6 +123,11 @@ export default function (Commands, Cypress, cy, state) {
         return subject
       }
 
+      // If the user callback returned a non-null value, we break cypress' subject chaining
+      // logic, so that we can use this subject as-is rather than the subject generated by
+      // any chainers inside the callback (if any exist).
+      cy.breakSubjectLinksToCurrentChainer()
+
       return ret
     }).catch(Promise.TimeoutError, () => {
       return $errUtils.throwErrByPath('invoke_its.timed_out', {
@@ -498,54 +465,16 @@ export default function (Commands, Cypress, cy, state) {
         $errUtils.throwErrByPath('each.invalid_argument')
       }
 
-      const nonArray = () => {
+      if (subject?.length === undefined) {
         return $errUtils.throwErrByPath('each.non_array', {
           args: { subject: $utils.stringify(subject) },
         })
       }
 
-      try {
-        if (!('length' in subject)) {
-          nonArray()
-        }
-      } catch (e) {
-        nonArray()
-      }
-
       if (subject.length === 0) {
         return subject
       }
 
-      // if we have a next command then we need to
-      // slice in this existing subject as its subject
-      // due to the way we queue promises
-      const next = state('current').get('next')
-
-      if (next) {
-        const checkSubject = (newSubject, args, firstCall) => {
-          if (state('current') !== next) {
-            return
-          }
-
-          // https://github.com/cypress-io/cypress/issues/4921
-          // When dual commands like contains() is used as the firstCall (cy.contains() style),
-          // we should not prepend subject.
-          if (!firstCall) {
-            // find the new subject and splice it out
-            // with our existing subject
-            const index = _.indexOf(args, newSubject)
-
-            if (index > -1) {
-              args.splice(index, 1, subject)
-            }
-          }
-
-          return cy.removeListener('next:subject:prepared', checkSubject)
-        }
-
-        cy.on('next:subject:prepared', checkSubject)
-      }
-
       let endEarly = false
 
       const yieldItem = (el, index) => {
@@ -573,11 +502,16 @@ export default function (Commands, Cypress, cy, state) {
 
       // generate a real array since bluebird is finicky and
       // doesnt want an 'array-like' structure like jquery instances
-      // need to take into account regular arrays here by first checking
-      // if its an array instance
       return Promise
       .each(_.toArray(subject), yieldItem)
-      .return(subject)
+      .then(() => {
+        // cy.each does *not* want to use any subjects that the user's callback generated - therefore we break
+        // cypress' subject chaining logic, which by default would override this with any subjects generated by
+        // the callback function.
+        cy.breakSubjectLinksToCurrentChainer()
+
+        return subject
+      })
     },
   })
 
diff --git a/packages/driver/src/cy/commands/misc.ts b/packages/driver/src/cy/commands/misc.ts
@@ -32,7 +32,7 @@ export default (Commands, Cypress, cy, state) => {
         const restoreCmdIndex = state('index') + 1
 
         cy.queue.insert(restoreCmdIndex, $Command.create({
-          args: [state('subject')],
+          args: [cy.currentSubject()],
           name: 'log-restore',
           fn: (subject) => subject,
         }))
diff --git a/packages/driver/src/cy/commands/querying/within.ts b/packages/driver/src/cy/commands/querying/within.ts
@@ -35,7 +35,9 @@ export default (Commands, Cypress, cy, state) => {
 
     fn.call(cy.state('ctx'), subject)
 
-    const cleanup = () => cy.removeListener('command:start', setWithinSubject)
+    const cleanup = () => {
+      cy.removeListener('command:start', setWithinSubject)
+    }
 
     // we need a mechanism to know when we should remove
     // our withinSubject so we dont accidentally keep 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()
+
     return subject
   }
 
diff --git a/packages/driver/src/cy/logGroup.ts b/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: [],
   })
 
   const forwardYieldedSubject = () => {
diff --git a/packages/driver/src/cypress.ts b/packages/driver/src/cypress.ts
@@ -218,6 +218,24 @@ class $Cypress {
     _.extend(this, browserInfo(config))
 
     this.state = $SetterGetter.create({}) as unknown as StateFunc
+
+    /*
+     * 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).
+     */
+    Object.defineProperty(this.state(), 'subject', {
+      get: () => {
+        $errUtils.warnByPath('subject.state_subject_deprecated')
+
+        return cy.currentSubject()
+      },
+    })
+
     this.originalConfig = _.cloneDeep(config)
     this.config = $SetterGetter.create(config, (config) => {
       if (this.isCrossOriginSpecBridge ? !window.__cySkipValidateConfig : !window.top!.__cySkipValidateConfig) {
@@ -640,9 +658,6 @@ class $Cypress {
       case 'cy:url:changed':
         return this.emit('url:changed', args[0])
 
-      case 'cy:next:subject:prepared':
-        return this.emit('next:subject:prepared', ...args)
-
       case 'cy:collect:run:state':
         return this.emitThen('collect:run:state')
 
diff --git a/packages/driver/src/cypress/chainer.ts b/packages/driver/src/cypress/chainer.ts
@@ -4,22 +4,13 @@ import $stackUtils from './stack_utils'
 export class $Chainer {
   specWindow: Window
   chainerId: string
-  firstCall: boolean
 
   constructor (specWindow) {
     this.specWindow = specWindow
     // The id prefix needs to be unique per origin, so there are not
     // collisions when chainers created in a secondary origin are passed
     // to the primary origin for the command log, etc.
     this.chainerId = _.uniqueId(`ch-${window.location.origin}-`)
-
-    // firstCall is used to throw a useful error if the user leads off with a
-    // parent command.
-
-    // TODO: Refactor firstCall out of the chainer and into the command function,
-    // since cy.ts already has all the necessary information to throw this error
-    // without an instance variable, in one localized place in the code.
-    this.firstCall = true
   }
 
   static remove (key) {
diff --git a/packages/driver/src/cypress/command_queue.ts b/packages/driver/src/cypress/command_queue.ts
@@ -15,8 +15,8 @@ import type { ITimeouts } from '../cy/timeouts'
 
 const debugErrors = Debug('cypress:driver:errors')
 
-const __stackReplacementMarker = (fn, ctx, args) => {
-  return fn.apply(ctx, args)
+const __stackReplacementMarker = (fn, args) => {
+  return fn(...args)
 }
 
 const commandRunningFailed = (Cypress, state, err) => {
@@ -165,9 +165,11 @@ export class CommandQueue extends Queue<$Command> {
         Cypress.once('command:enqueued', commandEnqueued)
       }
 
+      args = [command.get('chainerId'), ...args]
+
       // run the command's fn with runnable's context
       try {
-        ret = __stackReplacementMarker(command.get('fn'), this.state('ctx'), args)
+        ret = __stackReplacementMarker(command.get('fn'), args)
       } catch (err) {
         throw err
       } finally {
@@ -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)
+      cy.setSubjectForChainer(command.get('chainerId'), subject)
 
       return subject
     })
@@ -278,7 +280,8 @@ export class CommandQueue extends Queue<$Command> {
         })
 
         this.state('index', index + 1)
-        this.state('subject', command.get('subject'))
+
+        cy.setSubjectForChainer(command.get('chainerId'), command.get('subject'))
 
         Cypress.action('cy:skipped:command:end', command)
 
diff --git a/packages/driver/src/cypress/commands.ts b/packages/driver/src/cypress/commands.ts
diff --git a/packages/driver/src/cypress/cy.ts b/packages/driver/src/cypress/cy.ts
diff --git a/packages/driver/src/cypress/error_messages.ts b/packages/driver/src/cypress/error_messages.ts
diff --git a/packages/driver/src/cypress/log.ts b/packages/driver/src/cypress/log.ts
diff --git a/packages/driver/src/util/serialization/index.ts b/packages/driver/src/util/serialization/index.ts
diff --git a/scripts/run-webpack.js b/scripts/run-webpack.js
