Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Jul 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a3a9e35
Refactor chainer / Commands.add for readability
BlueWinds Jun 28, 2022
d3aef86
Fix invoking wrong function, add comment
BlueWinds Jun 28, 2022
5d81298
WIP, still failing tests
BlueWinds Jun 30, 2022
74fa0e5
Fix additional driver tests
BlueWinds Jun 30, 2022
916e6f1
Fix subject for utility commands
BlueWinds Jun 30, 2022
7021ba8
Better fix for utility functions
BlueWinds Jun 30, 2022
834ccae
Merge remote-tracking branch 'origin/develop' into issue-7306-addSele…
BlueWinds Jul 11, 2022
ac86823
Fix more tests
BlueWinds Jul 11, 2022
485a3fa
Another pass of test fixes
BlueWinds Jul 11, 2022
45689b6
Merge remote-tracking branch 'origin/develop' into issue-7306-addSele…
BlueWinds Jul 13, 2022
cf61e19
Additional comments, added shim to continue supporting cy.state('subj…
BlueWinds Jul 18, 2022
ec01d6d
Adjust reserved command names to no longer require manual addition / …
BlueWinds Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/driver/cypress/e2e/commands/assertions.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ describe('src/cy/commands/assertions', () => {
})

cy.get('body').then(() => {
expect(cy.state('subject')).to.match('body')
expect(cy.subjectForChainer(cy.state('chainerId'))).to.match('body')
})
})

Expand Down
3 changes: 2 additions & 1 deletion packages/driver/cypress/e2e/commands/commands.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
})
})
Expand Down
8 changes: 4 additions & 4 deletions packages/driver/cypress/e2e/commands/connectors.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.subjectForChainer(cy.state('chainerId'))).not.to.be.instanceof(this.remoteWindow.$)
})
})
})
Expand Down Expand Up @@ -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)
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/driver/src/cross-origin/origin_fn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.subjectForChainer(cy.state('chainerId')),
}, {
syncGlobals: true,
})
Expand Down
94 changes: 14 additions & 80 deletions packages/driver/src/cy/commands/connectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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', {
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
})
},
})

Expand Down
2 changes: 1 addition & 1 deletion packages/driver/src/cy/commands/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.subjectForChainer(state('chainerId'))],
name: 'log-restore',
fn: (subject) => subject,
}))
Expand Down
8 changes: 7 additions & 1 deletion packages/driver/src/cy/commands/querying/within.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Copy link
Member

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')

Copy link
Contributor Author

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.

Copy link
Member

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() ?

Copy link
Contributor Author

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.


return subject
}

Expand Down
1 change: 1 addition & 0 deletions packages/driver/src/cy/logGroup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export default (Cypress, userOptions: Cypress.LogGroup.Config, fn: Cypress.LogGr
const endLogGroupCmd = $Command.create({
name: 'end-logGroup',
injected: true,
args: [],
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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).

})

const forwardYieldedSubject = () => {
Expand Down
3 changes: 0 additions & 3 deletions packages/driver/src/cypress.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,9 +640,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')

Expand Down
9 changes: 0 additions & 9 deletions packages/driver/src/cypress/chainer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
13 changes: 8 additions & 5 deletions packages/driver/src/cypress/command_queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So current plan:

  1. Add a shim on cy.state('subject') in this PR so it keeps working as a chore. Merge this PR.
  2. In next PR, introduce selectors / function chains as subject, as a feat. Keep the shim working.
  3. Open a PR with cypress-testing-library to move them to the new functionality.
  4. Remove shim in breaking 11.x release eventually.

Copy link
Contributor Author

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().

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

cy.setSubjectForChainer(command.get('chainerId'), subject)

return subject
})
Expand Down Expand Up @@ -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)

Expand Down
5 changes: 0 additions & 5 deletions packages/driver/src/cypress/commands.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,6 @@ export default {
})
},

addSelector (name, fn) {
// TODO: Add overriding stuff.
return cy.addSelector(name, fn)
},

overwrite (name, fn) {
return storeOverride(name, fn)
},
Expand Down
Loading