Support running commands against the previous yielded subject

[kentcdodds]

I'm converting #100 by @tlrobinson to an issue. cc @twgraham, @intelligentspark, and @NicholasBoll.

Here's the original PR's comment (to save you a click)

What:

Changes the custom Cypress commands to respect the previously yielded subject, for example, with the following document:

    <button>Button Text 1</button>
    <div id="nested">
      <button>Button Text 2</button>
    </div>

this

cy.get('#nested').findByText('Button Text 1').should('not.exist')

would previously fail because findByText was not passed the result of cy.get('#nested') as its container (defaulting to document)

Why:

It's a convenient way to scope commands to the result of a previous command, and for users of builtin Cypress commands it's surprising that @testing-library/cypress doesn't currently behave like this.

How:

The prevSubject option is now passed to Cypress.Commands.add() (with a value of ['optional', 'document', 'element', 'window'] which matches builtin commands like contains. This causes the subject (if any) to be passed as the first argument to the command. The subject is given lower precedence than the testing-library's container option, but higher precedence than the document itself.

Checklist:

• Documentation N/A
• Tests
• Ready to be merged

Merging #100 caused a breaking change (#109). I've reverted it (0b73b69) and now we need to discuss whether we want to make this change and whether it's possible to solve the originally stated problem in the PR and have the testing style mentioned in #109.

[kentcdodds]

For me personally, I like to think of the commands I give to Cypress to be instructions I would give to a user, so chaining like this makes a lot of sense to me:

describe('anonymous calculator', () => {
  it('can make calculations', () => {
    cy.visit('/')               // visit this site
      .findByText(/^1$/)        // then find this element
      .click()                  // then click it
      .findByText(/^\+$/)       // then find this element
      .click()                  // then click it
      .findByText(/^2$/)        // then find this element
      .click()                  // then click it
      .findByText(/^=$/)        // then find this element
      .click()                  // then click it
      .findByTestId('total')    // then find this element
      .should('have.text', '3') // then assert it has this text
  })
})

I personally don't think that these queries make sense to scope each other (we already have that with the within command). The only reason I decided to allow #100 was because I thought it was just to support an additional style of writing these tests, not break the existing style which I think makes more sense.

Thoughts?

[tlrobinson]

I'm strongly in favor of this, despite it technically being a breaking change. I would prefer to match semantics of existing Cypress commands over supporting non-idiomatic style of writing tests.

IMHO the above is not idiomatic in Cypress, and isn't nearly as readable as:

    cy.visit('/')
    cy.findByText(/^1$/).click()
    cy.findByText(/^\+$/).click()
    cy.findByText(/^2$/).click()
    cy.findByText(/^=$/).click()
    cy.findByTestId('total').should('have.text', '3')

Cypress already does the work of queuing up commands that aren't chained, so chaining unrelated commands isn't necessary.

Cypress's builtin commands (e.x. contains which is similar to findByText) support using the previously yielded subject in chained commands (except get() and a couple others), so I think it's surprising that @testing-library/cypress's does not.

Cypress has end() specifically for yielding null which "resets" to the root element: https://docs.cypress.io/api/commands/end.html#Examples but they also state "Alternatively, you can always start a new chain of commands off of cy."

within is an option, but it's more verbose when all you want to do is something like cy.findByRole('dialog').findByText('submit') as you suggested here (and called it stellar!)

[kentcdodds]

Hi @tlrobinson,

Somehow I've managed to write non-idiomatic cypress for years and nobody told me 🙃

I just checked their kitchen sink example and boom:

  it('.check() - check a checkbox or radio element', () => {
    // https://on.cypress.io/check

    // By default, .check() will check all
    // matching checkbox or radio elements in succession, one after another
    cy.get('.action-checkboxes [type="checkbox"]').not('[disabled]')
      .check().should('be.checked')

    cy.get('.action-radios [type="radio"]').not('[disabled]')
      .check().should('be.checked')

    // .check() accepts a value argument
    cy.get('.action-radios [type="radio"]')
      .check('radio1').should('be.checked')

    // .check() accepts an array of values
    cy.get('.action-multiple-checkboxes [type="checkbox"]')
      .check(['checkbox1', 'checkbox2']).should('be.checked')

    // Ignore error checking prior to checking
    cy.get('.action-checkboxes [disabled]')
      .check({ force: true }).should('be.checked')

    cy.get('.action-radios [type="radio"]')
      .check('radio3', { force: true }).should('be.checked')
  })

Yeah, looks like when you want to change the subject of the chain, in this example at least, you should create a new cy chain.

This is unfortunate for me because I've been doing it differently so long and I've taught thousands of people to do it the way I do. But I think you're right and what you have described is probably better (it just means I have to re-record a bunch of cypress videos on testingjavascript.com 🤦‍♂️)

Ok, so for this, we're doing some breaking changes in the Testing Library realm in the next few weeks for other projects, so I think we could coordinate this and some other changes at the same time.

I'd love to hear other people's thoughts on this.

[kentcdodds]

@NicholasBoll, just commented in #109

I'll look up this issue and see if it is related to #100 and if there is any way to introduce #100 as a non-breaking change

That would be fantastic. Whatever we do, having an easier migration path to the new recommendation would be wonderful.

[NicholasBoll]

@tlrobinson is right, starting new chains is more idiomatic. I've taught people to think of a chain as a sentence with a complete subject. If I have to start with a new idea, I start a new sentence. I think it is easier to understand when I think of a chain as a complete thought.

cy.get is the only command that starts from scratch. I've talked with Brian before about this. He prefers the cy.get for new chains. He didn't go so far as to prevent cy.get from working while chained from a previous command.

There are get* commands that are currently just throwing errors if we want to designate those types of queries to always start from the root. cy.get() looks similar to cy.getByText... Perhaps that's a compromise? 🤷‍♂

I try to avoid cy.within for many of the reasons I avoid with in JavaScript. It is fine in your test code, but if you have a helper function that implicitly changes root scope, that's hard to debug. It can be nice to not scope multiple times though.

I think it is worthwhile to introduce a breaking change, but we'd have to communicate that. Chaining off previous commands and inheriting the previous element is how all other traversal commands work in Cypress.

This will delay #108, but that's okay.

[NicholasBoll]

I've verified #100 does indeed cause the issue in #109. I'll dig into why

[NicholasBoll]

Oh. I know why this is happening.

cy.visit is returning the Window and used as the previous subject to the first .findByText()

window.querySelectorAll is not a function. I've made some changes to how things work inside #108. I'll see if this can be non-breaking. I at least have a test case now

[kentcdodds]

Thanks for your work here Nicholas!

I wonder if we could make a codemod to update people's code automatically for them 🤔

[NicholasBoll]

My thought was to try to run the query scoped to the previous subject. If that fails, try running again unscoped with a warning message.

[NicholasBoll]

@kentcdodds The idea seems to work! It will first try scoping to the previous subject given by Cypress. If an error is encountered, it will record the error and then try with functionality previous to #100.

[kentcdodds]

That's fantastic! That way we can upgrade without the breaking change and update all the documentation to recommend a more idiomatic approach, and people can migrate over time. Then maybe eventually we can remove the old behavior.

[NicholasBoll]

I've updated #108 with the code. The code in #108 was primed for this type of change, so I tacked it on. The PR is kind of large. I could spend time breaking it down if we prioritize the features listed there. The code changes are interdependent so if I break it up, they would have to be done in a sequence.

[kentcdodds]

Amazing. Thank you @NicholasBoll! In a perfect world we'd probably just do the breaking change and move forward, but a lot of people rely on the current behavior so supporting them is important 👍

I'm fine putting it all in one PR. Normally like to have things split up a bit, but I'm not too concerned 🤷‍♂️