fix: Make querystrings consistent

[sainthkh]

• Partially addresses cy.request(), cy.visit() and cy.intercept() query strings are inconsistent #19407

User facing changelog

cy.visit() and cy.request generate the same querystring.

Additional details

• Why was this change necessary? => Given the same qs argument, cy.visit() and cy.request() created different querystrings.
• What is affected by this change? => N/A
• Any implementation details to explain? => I used mergeUrlWithParams function used in cy.visit() to generate querystring in cy.request. I decided it this way because cy.visit() behavior is consistent with the default URLSearchParams class.

How has the user experience changed?

Before:

cy.visit({ url: 'https://example.com', qs: { a: [1, 2], b: 3 } }) // => https://example.com?a=1%2C2&b=3
cy.request({ url: 'https://example.com', qs: { a: [1, 2], b: 3 } }) // => https://example.com?a%5B0%5D=1&a%5B1%5D=2&b=3

After:

cy.visit({ url: 'https://example.com', qs: { a: [1, 2], b: 3 } }) 
cy.request({ url: 'https://example.com', qs: { a: [1, 2], b: 3 } })
// Both => https://example.com?a=1%2C2&b=3

Why this PR doesn't close the issue?

AFAIK, there are 3 APIs in Cypress that use query string object. Before this PR, they all used the different libraries.

• cy.visit: url-parse, which uses querystringify
• cy.request: qs
• cy.intercept: default URLSearchParams class.

This is quite a mess because the standard library(URL and URLSearchParams classes) came out really late with Chrome 49 in 2016 while the libraries above are created in 2011(qs, nodejs default querystring), 2014(querystringify, url-parse).

And Cypress was created before 2016, it's natural that those libraries are used.

This PR deprecates qs in cy.request, and uses url-parse for both cy.visit and cy.request. It means cy.intercept still uses URLSearchParams.

I chose this way because there are 2 ways to merge parameters in URLSearchParams: append and set. I couldn't be sure mimicking current behavior is right.

Currently, when you provide parameters in both url and qs and names are conflicted, values in qs are used like below.

cy.visit({ url: '/?a=123&b=foo', qs: { a: 456 } }) // => /?a=456&b=foo

(It's happening because we simply merge objects like below:)

cypress/packages/driver/src/cypress/location.ts

Lines 160 to 165 in 0143e13

	static mergeUrlWithParams (url, params) {
	url = new UrlParse(url, null, true)
	url.set('query', _.merge(url.query || {}, params))
	return url.toString()
	}

But when we use URLSearchParams, we can preserve both of them with append method like below:

const params = new URLSearchParams({ a: 123, b: 'foo' })
params.append('a', 456)
console.log(params.toString()) // a=123&b=foo&a=456

I think it is the better behavior. So, I decided to stop this PR here and seek your opinions about this.

PR Tasks

• Have tests been added/updated?
• [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [na] Has a PR for user-facing changes been opened in cypress-documentation?
• [na] Have API changes been updated in the type definitions?
• [na] Have new configuration options been added to the cypress.schema.json?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[jennifer-shehane]

@sainthkh This looks like this change would be a breaking change, correct?

[sainthkh]

@jennifer-shehane It's somewhere between.

If we define qs behavior is correct, cy.request's behavior is also right, but different. In this case, it's a breaking change.

If we define qs behavior is wrong because it doesn't match URLSearchParams's behavior, then cy.request's behavior is also wrong. In this case, it's a bug.

[jennifer-shehane]

@sainthkh Thanks for the clarification. I believe we'll need to sync on how we plan to proceed. Right now we don't have as much time to dedicate to review of this, so please be patient.

[BlueWinds]

I like the change - the consistency is valuable, but do think it's breaking. Currently proposing to the team that we merge this when we're ready to release another breaking change (but not cause 11.x immediately for this purpose alone).

[BlueWinds]

We have a merge conflict, but otherwise looks good to me, with tests covering the changes.

[rachelruderman]

I ❤️ consistency, thank you for the contribution and very detailed description!

[cypress]

---

Test summary

37689 • 0 • 456 • 0 • 9

---

Run details

Project	cypress
Status	Passed
Commit	6ccb1ce
Started	Jun 27, 2022 11:48 PM
Ended	Jun 28, 2022 12:05 AM
Duration	17:28 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	actions/click.cy.js		2
1	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
2	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
	xhr.cy.js		3
1	... > logs Method, Status, URL, and XHR
2	... > logs response
3	... > logs response
This comment includes only the first 5 flaky tests. See all 9 flaky tests in the Cypress Dashboard.

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[cypress-bot]

Released in 12.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.0.0, please open a new issue.