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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX release] Don't serialize default QPs on RouterService #19971

Merged
merged 1 commit into from Feb 18, 2022
Merged

[BUGFIX release] Don't serialize default QPs on RouterService #19971

merged 1 commit into from Feb 18, 2022

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented Feb 14, 2022 •
edited

Fixes #19493.

@wagenet wagenet force-pushed the router-service-default-qps branch 2 times, most recently from b9fa54d to 05f2a12 Compare February 15, 2022 00:16
@wagenet wagenet changed the title Add failing test for router service default QPs [BUGFIX release] Don't serialize default QPs on RouterService Feb 15, 2022
NullVoxPopuli
NullVoxPopuli reviewed Feb 15, 2022
View reviewed changes
packages/@ember/-internals/routing/lib/services/router.ts
// Spoilers: under the hood this currently uses router.js APIs which
// *do not* account for this being `undefined`.
routeName as string,
targetRouteName,
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

what happens when targetRouteName here is undefined?

should we remove the as and instead narrow the type with an assert?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t know. It was like this before and I just moved things around. I do want to review all as casts someone soon.

@locks locks requested review from ef4 and wycats February 18, 2022 17:21
@wagenet wagenet merged commit e904e25 into emberjs:master Feb 18, 2022
@wagenet wagenet deleted the router-service-default-qps branch February 22, 2022 18:24
@snewcomer
Copy link
Contributor

Thanks for doing this!!

@rwjblue
Copy link
Member

rwjblue commented Mar 1, 2022 •
edited

I think backporting this to beta is probably fine, but I'd hold off on backporting to release. This is very likely to accidentally break things for folks not even realizing they were relying on the prior behavior. I definitely think the new behavior is correct,
it's just more risky than a patch release IMO.

@boris-petrov
Copy link
Contributor

I updated Ember from 4.2.0 to 4.3.0 and a test started failing. I have a component that does:

this.router.replaceWith(...this.model.routeComponents, { queryParams: this.model.queryParameters })

Where this.model.routeComponents is ['some-route', 'some parameter'] and this.model.queryParameters is undefined. This used to work fine before. After the update:

More context objects were passed than there are dynamic segments for the route: some-route

It's because of the "empty" queryParams parameter. If I remove it, it works. Is that what this PR fixes or is this a regression?

@boris-petrov
Copy link
Contributor

@wagenet, @rwjblue, @snewcomer, @locks, @NullVoxPopuli - sorry for pinging you all but any ideas about my comment above?

@boris-petrov
Copy link
Contributor

Actually the same issue happens with this.router.replaceWith(...this.model.routeComponents, {}) which definitely is a bug I believe.

@NullVoxPopuli
Copy link
Sponsor Contributor

@boris-petrov
sorry for taking so long to reply, can you provide the definitions of routeComponents and querpParameters?

@NullVoxPopuli
Copy link
Sponsor Contributor

which definitely is a bug I believe.

I'd agree with that -- {} should behave the same as not passing it.

want to submit a PR?

@boris-petrov boris-petrov mentioned this pull request Apr 1, 2022
Closed
@boris-petrov
Copy link
Contributor

sorry for taking so long to reply, can you provide the definitions of routeComponents and querpParameters?

No worries. Well, routeComponents is just a list of strings and queryParameters is a hash (but in this case undefined).

want to submit a PR?

It's probably going to be difficult for me, I'm completely unfamiliar with the code. But I did add a PR with tests.

@wagenet
Copy link
Member Author

wagenet commented Apr 1, 2022

@boris-petrov Thanks for the PR! I've added my comments there.

@Turbo87 Turbo87 mentioned this pull request Apr 6, 2022
Closed
sergiofenoll added a commit to kanselarij-vlaanderen/frontend-kaleidos that referenced this pull request Apr 16, 2022
@sergiofenoll
Tweak default sorting of agendas table & QP error
164047d 
The session-kind test expects the PVV Ministerraad to come after the
normal Ministerraad in a filtered agendas table. With this new
default filtering, we explicitly apply this ordering.

An agendaitem delete call was causing errors which might be related to
these Ember bugs:

- emberjs/ember.js#20051
- emberjs/ember.js#19971

Leaving out setting anchor to null in hope that it might fix it.
nevilm-lt pushed a commit to nevilm-lt/ember.js that referenced this pull request Apr 22, 2022
@wagenet @nevilm-lt
Merge pull request emberjs#19971 from wagenet/router-service-default-qps
404f550 
[BUGFIX release] Don't serialize default QPs on RouterService
nevilm-lt pushed a commit to nevilm-lt/ember.js that referenced this pull request Apr 22, 2022
@wagenet @nevilm-lt
Merge pull request emberjs#19971 from wagenet/router-service-default-qps
9369e96 
[BUGFIX release] Don't serialize default QPs on RouterService
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NullVoxPopuli NullVoxPopuli NullVoxPopuli left review comments

@chriskrycho chriskrycho chriskrycho left review comments

@locks locks locks approved these changes

@ef4 ef4 Awaiting requested review from ef4

@wycats wycats Awaiting requested review from wycats

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Router service transitionTo adds unspecified query params (with their default values)
7 participants
@wagenet @snewcomer @rwjblue @boris-petrov @NullVoxPopuli @locks @chriskrycho