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

scrollIntoView doc page contains unsafe chains in examples #5002

Open
JessefSpecialisterren opened this issue Jan 26, 2023 · 3 comments · May be fixed by #5161
Open

scrollIntoView doc page contains unsafe chains in examples #5002

JessefSpecialisterren opened this issue Jan 26, 2023 · 3 comments · May be fixed by #5161

Comments

@JessefSpecialisterren
Copy link
Contributor

Subject

api

Description

https://docs.cypress.io/api/commands/scrollintoview states that "[i]t is unsafe to chain further commands that rely on the subject after .scrollIntoView()." However, two examples on the page chain .should('be.visible') on scrollIntoView. I think the examples should be updated to re-query the DOM before doing that assertion

Pinging @BlueWinds (hope that's an okay thing to do?) to check if this is indeed a desired documentation change

@BlueWinds
Copy link
Contributor

@JessefSpecialisterren - Yep, looks like we should update the examples here. If you'd like to submit a PR to update them, feel free to ping me there as well - otherwise, I'll see if I have a few minutes to get to it next week.

@JessefSpecialisterren
Copy link
Contributor Author

@BlueWinds I went and got started on fixing this, but ran into several things:

  1. One of the examples for scrollIntoView comes with a screenshot of the Command Log, which seems to come from cypress-example-kitchensink. I looked at that project and realized there were tons of unsafe chains in there
  2. I took a closer look at cypress-documentation and found unsafe chains in several more examples all over the place
  3. I looked up cypress-realworld-app and found lots of unsafe chains in there as well

This change in how commands are handled seems to have quite the avalanche of consequences 😅. I can't justify spending a lot of time on this; our project has a huge backlog, and for our purposes it's enough to know that Cypress examples are going to contain unsafe chains for a while

For the moment, I can offer the following:

  • The suggestion to create issues about unsafe chains in the docs and examples repositories, so it's officially a known issue
  • The regex I built to detect unsafe chains in our project:

\.(blur|check|clear|click|dblclick|each|focus|go|pause|reload|rightclick|screenshot|scrollIntoView|scrollTo|select|selectFile|spread|submit|then|trigger|type|uncheck|wait|within)\(([^()]|\n|\r|\(([^()]|\n|\r|\(([^()]|\n|\r)*\))*\))*\)[\n\r\s]*\.

Caveats:

  • It's built for and tested in vscode
  • It's hardcoded to support up to three levels of nested parentheses
  • Depending on the code, it can find a lot of false positives

And I wanted to add that I appreciate the hard work and effort you've been putting into this change 🙂! I imagine you got a lot more than you bargained for when you started on it, but you've already moved a mountain and seem to be in the process of moving another one. Thank you for making this long-requested improvement happen!

@emilyrohrbough emilyrohrbough linked a pull request Mar 31, 2023 that will close this issue
@emilyrohrbough
Copy link
Member

@JessefSpecialisterren oh boy! This was a bit of a miss on our part on updating our examples that did this. The docs are harder to catch since we aren't running these tests 😅 Please, as you see examples, feel free to update and/or log issues for smaller pieces. Trying to do everything yourself could likely take quite a bit of time and totally understand you have other things as well. T

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants