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

fix: cy.request allows nested objects as qs #27908

Closed
wants to merge 17 commits into from

Conversation

S-Tornqvist
Copy link
Contributor

@S-Tornqvist S-Tornqvist commented Sep 26, 2023

Additional details

cy.request() now correctly parses nested objects and arrays for the query string (qs), which is the intended behavior. The behavior was lost in 12.0.0 (see #27745). As the qs functionality is shared between cy.visit() and cy.request(), the behavior of cy.visit() also changes.

Note that the behavior only changes when using nested objects for the query string. Previously these would be parsed as "[object Object]", so no useful behavior is lost.

Steps to test

Example usage:

cy.request({
    url: "http://localhost:3000",
    qs: { foo: { bar: "baz" } }
});

How has the user experience changed?

na

PR Tasks

@CLAassistant
Copy link

CLAassistant commented Sep 26, 2023

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

@S-Tornqvist S-Tornqvist changed the title add driver/qs test for nested objects+arrays resolve #27745: qs parse nested objects and arrays correctly Sep 26, 2023
@S-Tornqvist S-Tornqvist changed the title resolve #27745: qs parse nested objects and arrays correctly fix: request command allows nested objects as qs Sep 27, 2023
@jennifer-shehane
Copy link
Member

@S-Tornqvist Does this introduce a breaking change? I'd like tests around the change in visit behavior. Let us know when this is ready for review.

@S-Tornqvist S-Tornqvist force-pushed the issue-27745 branch 2 times, most recently from ddb9efd to d6b8489 Compare September 27, 2023 14:10
@S-Tornqvist
Copy link
Contributor Author

@S-Tornqvist Does this introduce a breaking change? I'd like tests around the change in visit behavior. Let us know when this is ready for review.

Hi @jennifer-shehane,

I'm pretty new here so I was gonna finalize the draft and then ask some questions regarding the dependencies, changelog etc. I'm struggling a bit with the semantic pr check, but I'm getting there :) No breaking changes introduced, so I'll set the version to 13.3.1. Thanks for the heads up.

@S-Tornqvist S-Tornqvist marked this pull request as ready for review October 9, 2023 10:01
@S-Tornqvist
Copy link
Contributor Author

S-Tornqvist commented Oct 9, 2023

@jennifer-shehane the pr is now ready for review. Tests for nested querystrings has been added to cy.request and cy.visit. Only the behavior for nested querystrings should be affected, and as these were previously serialized as "[object Object]" no useful behavior should be lost. Previous qs tests proves that non-nested behavior is unchanged

@S-Tornqvist S-Tornqvist changed the title fix: request command allows nested objects as qs fix: cy.request allows nested objects as qs Oct 11, 2023
@S-Tornqvist
Copy link
Contributor Author

@jennifer-shehane @AtofStryker @nagash77 could you review the pr?

@S-Tornqvist
Copy link
Contributor Author

Is there anything I can do more or did wrong here? @jennifer-shehane

@AtofStryker
Copy link
Contributor

@S-Tornqvist sorry for the delay in response here as I have been out of office for a few months. From a glance, the code looks fine and don't think it should introduce a breaking change as it is just serializing the nested object. I am going to pull from develop and get this PR running again!

@S-Tornqvist
Copy link
Contributor Author

@S-Tornqvist sorry for the delay in response here as I have been out of office for a few months. From a glance, the code looks fine and don't think it should introduce a breaking change as it is just serializing the nested object. I am going to pull from develop and get this PR running again!

Thanks! I pulled develop and resolved the changelog conflict

@AtofStryker
Copy link
Contributor

@S-Tornqvist looks like there is still a conflict?

@emilyrohrbough emilyrohrbough added the type: breaking change Requires a new major release version label Jan 8, 2024
@emilyrohrbough
Copy link
Member

@jennifer-shehane @AtofStryker Changing the query parsing dependency would indeed be a breaking change. It would change the results of the parsed query people are expecting as a response. The cy.visit(), cy.intercept() and cy.request() query logic are not aligned, see: #19407.

@AtofStryker
Copy link
Contributor

@jennifer-shehane @AtofStryker Changing the query parsing dependency would indeed be a breaking change. It would change the results of the parsed query people are expecting as a response. The cy.visit(), cy.intercept() and cy.request() query logic are not aligned, see: #19407.

Yeah this unfortunately seems to be the case 🙁 . I relooked and tracked my comment back to #20302 and missed the breaking change label. Sounds like we might need a broader strategy to align parsed queries through cy.visit(), cy.intercept() and cy.request() and verify complex query structures are serialized correctly and release in a major version?

@cypress-app-bot
Copy link
Collaborator

This PR has not had any activity in 180 days. If no activity is detected in the next 14 days, this PR will be closed.

@cypress-app-bot cypress-app-bot added the stale no activity on this issue for a long period label Jul 7, 2024
@cypress-app-bot
Copy link
Collaborator

This PR has been closed due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale no activity on this issue for a long period type: breaking change Requires a new major release version
Projects
None yet
6 participants