cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
47.02k stars 3.18k forks source link

Convert cy.then to a child command #466

Closed brian-mann closed 3 years ago

brian-mann commented 7 years ago

Currently cy.then operates as a dual command so it can be used like this..

cy.then(() => {

})

This is essentially an anti-pattern, and its super confusing from anyone coming from Promises. It makes no sense not to yield a subject.

Attaching it as a child command does yield a subject.

cy
  .wrap("foo")
  .then((str) => {
    expect(str).to.eq("foo") // true
  })

But calling it as the first chained command will always prevent it from yielding a subject even if the previous command returned one.

cy.wrap("foo")
cy.then((str) => {
  expect(str).to.be.null // true
})

By switching cy.then to a child command we will avoid this anti-pattern + confusion, and it will instead throw when attempting to be used as a parent command.

You will need to enforce a parent command before calling it, so even if you don't care about the subject you'll need to do something like this

// wrap null but at least now its clear!
cy.wrap(null).then((ret) => {

})

// this is analogous to
Promise.resolve(null).then((ret) => {

})
jennifer-shehane commented 5 years ago

Adding code where this exists as well as notes https://github.com/cypress-io/cypress/blob/issue-3267/packages/driver/src/cy/commands/connectors.coffee#L346

## temporarily keeping this as a dual command
## but it will move to a child command once
## cy.resolve + cy.wrap are upgraded to handle
## promises

@brian-mann @Bkucera Is this ready for work?

kuceb commented 5 years ago

@jennifer-shehane I believe it is

dwelle commented 4 years ago

This is essentially an anti-pattern, and its super confusing from anyone coming from Promises. It makes no sense not to yield a subject.

I actually disagree with this, and would argue to keep it as a dual command, and even promote this use case at first mention of cy.then in the docs.

Why? Exactly because people tend to mistake Cypress' .then for promise chaining, while it's anything but. People have no idea that .then is actually a command, and instead think it's a promise thing, which only fuels the confusion of why we can't use async/await.

If we instead start promoting the idea that cy.then is a command like any other, it might not take as much time and effort for people to stop thinking in promises, and start thinking in chains (or observables, or whatever paradigm we'd like to explain Cypress as).

IMO, giving the cy.then command the then name only added to the confusion to begin with, but what's done is done.

Barring the above, I still don't think using cy.then to start off a chain is an anti-pattern. It's simply enqueueing a callback to be run next, and it makes sense to use it as such (and is more expressive than doing cy.wrap(null).then --- if I saw that in code, I'd not know what the hell that's about).

There are cases when you need to read an async value, and you can't alias it.

jennifer-shehane commented 3 years ago

Since this issue hasn't had activity in a while, we'll close the issue. Please comment if there is new information to provide concerning the original issue and we'd be happy to reopen.