[BUGFIX] - Merge query params from previous transition into active transition by raytiley · Pull Request #13273 · emberjs/ember.js

raytiley · 2016-04-07T01:33:45Z

This merges in queryParams from an active transition in such a way that prepareQueryParams won't duplicate them.

It also won't merge in the qp if it is already int he qp changed list, for example if it's already been set during route setup by a call to controller.set('qpProp', 'someValue').

Fixing these things with my current test jsbin also recreated another issue that has been biting people and that is that a call to transitionTo creates an aborted transition that calls didTransition with an empty set of handlerInfos. This would cause the updatePaths method to fail since it couldn't calculate the paths without the handler infos. We now bail early from updatePaths it is called with an empty array of handlerInfos.

I think this is fine because the aborted transition is followed by a successful transition that will update the paths appropriatly.

Still todo - TESTS

rwjblue · 2016-04-07T02:31:40Z

packages/ember-routing/lib/system/router.js

Can this bit of code live in a separate method (_processActiveTransitionQueryParams or something)?

rwjblue · 2016-04-10T15:56:44Z

Thanks for working on this @raytiley, let me know if I can help...

raytiley · 2016-04-10T16:28:05Z

Come to Maine and babysit for a few hours? I should be able to wrap this
up tonight.

On Sunday, April 10, 2016, Robert Jackson notifications@github.com wrote:

Thanks for working on this @raytiley https://github.com/raytiley, let
me know if I can help...

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#13273 (comment)

rwjblue · 2016-04-10T16:34:10Z

Come to Maine and babysit for a few hours? I should be able to wrap this up tonight.

Sounds fun! Might have to make a pit stop at the Bissell Brothers though....

knownasilya · 2016-04-15T19:49:12Z

@raytiley I'm up to help any way I can, although visiting to babysit might be out of the question.

igorT · 2016-04-19T19:32:13Z

packages/ember-routing/lib/system/router.js

Can you please break this down and add more local vars to make this more readable

raytiley · 2016-04-19T19:34:27Z

Hey Everyone... sorry. I've been really busy at work. I promise I will clean this up / get tests this week.

igorT · 2016-04-19T21:40:47Z

Nevermind, it was due to unrelated route recognizer bug.

I've been testing this PR in our app and it seems to fix certain query param issues that broke in 2.4.4 but introduces another bug where initial load of a page with a query 404s. Investigating

igorT · 2016-04-19T22:04:47Z

packages/ember-routing/lib/system/router.js

This is unused

raytiley · 2016-04-20T02:58:32Z

First steps... I rebased the code and fixed the jshint error. Tomorrow I'll write some tests based on some jsbin cases I used to write this code. Sorry for the delay.

igorT · 2016-04-21T02:51:12Z

@raytiley I added some tests here for things that worked in 2.4.3 and broke in 2.4.4 I think your branch fixed them as of yesterday.
53fcdbc

pixelhandler · 2016-04-22T02:23:24Z

packages/ember/tests/routing/query_params_test.js

This test looks good!

Seems to me that this test will fail with "Uncaught Error, Assertion Failed: You're not allowed to have more than one controller property map to the same query param key…" without the code to fix.

lolmaus · 2016-04-26T13:09:04Z

This issue needs to be properly tagged. At least with Query Params.

lolmaus · 2016-04-26T13:19:23Z

@Sinled has discovered a regression with an array query param.

Previously, you could have an array query param like this:

export default Ember.Controller.extend({
  queryParams: ['fields',],
  fields: ['field1', 'field2']
})

Ember would properly (and transparently!) serialize them into strings and then deserialze back.

After the regression, array query params are not deserialized and the Controller's array property gets rewritten with a string.

Me and @Sinled managed to narrow down the regression to this commit by @raytiley: master 2b42d56, lts-2-4 7460b68.

Please tell us whether:

this is a bug or works as designed;
this is a known or new issue;
it should be reported as a new GitHub issue ticket.

rwjblue · 2016-04-26T14:06:40Z

This issue needs to be properly tagged.

This comment is not helpful, please keep things constructive.

Please tell us whether:

this is a bug or works as designed;

Bug.

this is a known or new issue;

If you didn't find it via a GitHub search, it is a new issue. I personally do not recall an issue opened about 2.4.4 regarding array query params.

it should be reported as a new GitHub issue ticket.

If you didn't find it via a GitHub search, it is a new issue. Please test this PR to see if it fixes.

mattmarcum · 2016-04-28T17:40:42Z

packages/ember-routing/lib/system/router.js

I'm running into a similar problem but in my case infos isn't even an array. I did something like:

import { isArray } from 'ember-runtime/utils'; ... if (!isArray(infos) || infos.length === 0) { return; }

homu · 2016-05-06T03:55:43Z

☔ The latest upstream changes (presumably b125ff5) made this pull request unmergeable. Please resolve the merge conflicts.

knownasilya · 2016-05-09T19:17:55Z

I'm totally getting bit by the QPs not being deserialized properly (arrays) in the queryParamsDidChange hook. How's this PR going, and would it solve that?

pixelhandler · 2016-05-13T16:40:51Z

@stefanpenner @chancancode question for you, do we need a "regression" label on this?

pixelhandler · 2016-05-13T16:45:42Z

@raytiley thanks for putting this PR into play. This will help people that would like to adopt the LTS release. I spoke to a few people at our meetup Wed. And noticed those interested in LTS are running 2.4.3 instead of 2.4.5 due to #13258

lolmaus · 2016-06-05T18:50:47Z

Related: #13591

Sinled · 2016-06-14T13:34:23Z

2.4.6 fixed my problem with query params serialisation, but it still present in 2.6.0

rwjblue · 2016-06-14T13:38:07Z

Confirm. This PR is still needed.

@igorT - Were you going to pick this up?

homu · 2016-06-15T20:00:11Z

☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts.

This merges in queryParams from an active transition in such a way that `prepareQueryParams` won't duplicate them. It also won't merge in the qp if it is already int he qp changed list, for example if it's already been set during route setup by a call to `controller.set('qpProp', 'someValue')`. Fixing these things with my current test jsbin also recreated another issue that has been biting people and that is that a call to `transitionTo` creates an aborted transition that calls `didTransition` with an empty set of handlerInfos. This would cause the `updatePaths` method to fail since it couldn't calculate the paths without the handler infos. We now bail early from `updatePaths` it is called with an empty array of handlerInfos. I think this is fine because the aborted transition is followed by a successful transition that will update the paths appropriatly. This gets the active queryParams and the provided using the same key `controller:prop` so that the calls to `assign` override as expected. This fixes issues where the QP doesn't update properly on refresh because the merged value conflicts with the provided value.

rwjblue · 2016-06-27T13:58:40Z

🎉

nathanhammond · 2016-08-10T06:55:04Z

Hey everybody who paid attention to this thread! We've a few open bugs likely triggered by this change. Anybody have time to dive in?

This was referenced Apr 7, 2016

Assertion Failed: You're not allowed to have more than one controller property map to the same query param key #13258

Closed

Query Params Reset to Default Values on Initial Render If transitionTo is called in beforeModel #12169

Closed

rwjblue reviewed Apr 7, 2016
View reviewed changes

raytiley mentioned this pull request Apr 8, 2016

QP does not get updated when model is refreshed #13263

Closed

rwjblue mentioned this pull request Apr 13, 2016

transitionTo with queryParams fails for renamed params #11977

Closed

igorT reviewed Apr 19, 2016
View reviewed changes

packages/ember-routing/lib/system/router.js Outdated

Copy link

Member

igorT Apr 19, 2016

Choose a reason for hiding this comment

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

This is unused

raytiley force-pushed the moar-qp-fixes branch from a07a9ad to 64cd2cc Compare April 20, 2016 02:41

pixelhandler reviewed Apr 22, 2016
View reviewed changes

mattmarcum mentioned this pull request Apr 28, 2016

{{link-to}} with model and query params and target route with refreshModel: true errors #13395

Closed

mattmarcum reviewed Apr 28, 2016
View reviewed changes

mattmarcum mentioned this pull request Apr 28, 2016

[BUGFIX beta] fixes transitionTo failing when called in beforeModel hook #13426

Closed

pixelhandler added the Query Params label May 13, 2016

lolmaus mentioned this pull request Jun 5, 2016

_array_ queryParam converted to _string_ after transitionTo #13591

Closed

acorncom mentioned this pull request Jun 10, 2016

Unable to transition to child route with mapped query parameters #13612

Closed

fivetanley mentioned this pull request Jun 15, 2016

[BUGFIX] normalize query params when merging in from active transition #13682

Closed

raytiley force-pushed the moar-qp-fixes branch from bc68607 to adef2d0 Compare June 27, 2016 13:02

raytiley and others added 2 commits June 27, 2016 09:20

Add renetrant test for query params changes

f693865

raytiley force-pushed the moar-qp-fixes branch from adef2d0 to f693865 Compare June 27, 2016 13:20

raytiley changed the title ~~[WIP] - Fixing query params~~ [BUGFIX] - Merge query params from previous transition into active transition Jun 27, 2016

rwjblue merged commit 7e62fb0 into emberjs:master Jun 27, 2016

rwjblue deleted the moar-qp-fixes branch June 27, 2016 13:58

raytiley mentioned this pull request Jun 27, 2016

Possible Regression: Query params alias behavior changed? #12102

Closed

joepstender mentioned this pull request Jul 10, 2016

Adding loans as a nested resource inside friends show triggers an error abuiles/ember-101#90

Closed

cibernox mentioned this pull request Jul 29, 2016

Transitions to the same route with different model + QPs throws Error #13954

Closed

alisdair mentioned this pull request Aug 3, 2016

Exception thrown from _fullyScopeQueryParams #14010

Closed

Uh oh!

Conversation

raytiley commented Apr 7, 2016

Uh oh!

rwjblue Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

rwjblue commented Apr 10, 2016

Uh oh!

raytiley commented Apr 10, 2016

Uh oh!

rwjblue commented Apr 10, 2016

Uh oh!

knownasilya commented Apr 15, 2016

Uh oh!

igorT Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

raytiley commented Apr 19, 2016

Uh oh!

igorT commented Apr 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorT Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

raytiley commented Apr 20, 2016

Uh oh!

igorT commented Apr 21, 2016

Uh oh!

pixelhandler Apr 22, 2016

Choose a reason for hiding this comment

Uh oh!

lolmaus commented Apr 26, 2016

Uh oh!

lolmaus commented Apr 26, 2016

Uh oh!

rwjblue commented Apr 26, 2016

Uh oh!

mattmarcum Apr 28, 2016

Choose a reason for hiding this comment

Uh oh!

homu commented May 6, 2016

Uh oh!

knownasilya commented May 9, 2016

Uh oh!

pixelhandler commented May 13, 2016

Uh oh!

pixelhandler commented May 13, 2016

Uh oh!

lolmaus commented Jun 5, 2016

Uh oh!

Sinled commented Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwjblue commented Jun 14, 2016

Uh oh!

homu commented Jun 15, 2016

Uh oh!

rwjblue commented Jun 27, 2016

Uh oh!

nathanhammond commented Aug 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

igorT commented Apr 19, 2016 •

edited

Loading

Sinled commented Jun 14, 2016 •

edited

Loading