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.

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

chore: add deprecation warning to query* #125

Conversation

NicholasBoll
Copy link
Contributor

@NicholasBoll NicholasBoll commented Mar 5, 2020

Fixes #117

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

This PR is overdue. I had the fix for a while, but sat on it too long. If you read up on #117, you'll see the person who filed the bug no longer has the issue. This PR does revert the query* functionality to what it was previously. The functionality difference is subtle, but I'll explain it here:

After #108, query* was subtly changed to have the same functionality as find* queries, but with no retries. This changed the error messages (they were more helpful), but also changed the way the command worked which would break in this circumstance:

cy.queryByText('Never Exists').then(val => {
  cy.wrap(val).should('not.exist')
})

Before the cy.queryByText('Never Exists') would return null and the test would pass. After, the cy.queryByText('Never Exists') ran through Cypress's internal verification of upcoming assertions. Implicitly a .should('exist') was added to all commands that didn't explicitly have an assertion, making the command run like the following:

cy.queryByText('Never Exists').should('exist').then(val => {
  cy.wrap(val).should('not.exist')
})

With this subtle change, immediate non-existence no longer works with query* because it is first expected to exist and will fail before the .then is called.

There is a deprecation warning that states query* should not be used and to use find* instead. The next major release will remove query*. Within the context of Cypress, only the find* line of queries make sense. All Cypress commands retry since all Cypress commands are asynchronous and query* was only useful because find* didn't handle eventual non-existence well.

@NicholasBoll
Copy link
Contributor Author

This PR navigates breaking changes over minor versions with time in-between. The original author fixed it so this PR doesn't matter to him anymore. We're not sure which breaking API is what people are using...

So question on this PR. Should I:

  1. Keep the PR as-is. This will be a breaking change for those relying on the behavior of query* for immediate non-existence passed to a .then. This is probably rare, but who knows. We've been wrong before.

  2. Change the PR, but keep the deprecation warning. This will not be a breaking change for those on the latest version already. Nobody else has complained about a breaking change. The original author has resolved by updating his code.

Personally, I like 2.

@kentcdodds
Copy link
Member

I'm in favor of option 2 as well.

And, as a side-note, if there are any other breaking changes you'd like to make, then feel free to open them on the beta branch. Now is the perfect time. I'm thinking we'll merge that to master and get a real release in the next few days.

@NicholasBoll NicholasBoll changed the title fix: Revert to previous query* API chore: add deprecation warning to query* Mar 5, 2020
@NicholasBoll
Copy link
Contributor Author

Updated with option 2.

@kentcdodds
Copy link
Member

This looks great!

Here's the build failure:

image

@NicholasBoll
Copy link
Contributor Author

It seems like it might be a timing thing. It passes locally. I'll update for timing differences.

@kentcdodds
Copy link
Member

Hmmm... Still failing on CI 😬

@NicholasBoll
Copy link
Contributor Author

It was passing locally no matter what I did. Upgrading to 4.1.0 allowed me to duplicate CI failures. Very strange.

@NicholasBoll
Copy link
Contributor Author

It looks like an upgrade to Chai v4: chaijs/chai#1007 (comment)

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is great. I think we can meet this and then drop query for the major version tomorrow. Thank you for all your work!

@kentcdodds kentcdodds merged commit ab34b09 into testing-library:master Mar 12, 2020
@kentcdodds
Copy link
Member

🎉 This PR is included in version 5.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@NicholasBoll NicholasBoll deleted the pr/revert-breaking-query-changes branch March 12, 2020 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert changes to query*s and add deprecation warning
2 participants