cypress-documentation icon indicating copy to clipboard operation
cypress-documentation copied to clipboard

scrollIntoView doc page contains unsafe chains in examples

Open JessefSpecialisterren opened this issue 2 years ago • 3 comments

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

JessefSpecialisterren avatar Jan 26 '23 10:01 JessefSpecialisterren

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

BlueWinds avatar Jan 26 '23 16:01 BlueWinds

@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!

JessefSpecialisterren avatar Jan 27 '23 12:01 JessefSpecialisterren

@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

emilyrohrbough avatar Mar 31 '23 13:03 emilyrohrbough