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

bug(router): Allowing multiple query params of the same name to be se… #11373

Closed

Conversation

spacebeers
Copy link
Contributor

What kind of change does this PR introduce? (check one with "x")

[ x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

Currently when going to a url with multiple query parameters with the same key the DefaultUrlSerializer is removing all but the last one from the url.

/something/route?room=a1&room=a4&room=a6

This link will be rewritten to:

/something/route?room=a6

What is the new behavior?

Links with multiple query parameters will be left in the url as is and the parameters will be serialized into an array.

/something/route?room=a1&room=a4&room=a6

room["a1", "a4", "a6"]

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x ] No

…rialized into an array

The current router will remove all but the last one if the url contains multiple query params of the same name.

`router/foo?a=1&a=2&a=5` will be rewritten as `router/foo?a=5`
@vicb vicb added area: router action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 6, 2016
@severinkehding
Copy link

I would really appreciate it if this fix would find its way into a version anytime soon :(

@spacebeers
Copy link
Contributor Author

@severinkehding we've had to bodge it on our site.

var paths = {
        "*url_serializer.js": "dist/our-app/routes/default.url.serializer.js"
    };

We've ran the new version and tell System JS to use that instead. Not idea but it works for now.

@vsavkin
Copy link
Contributor

vsavkin commented Oct 19, 2016

Do I understand it correctly that /something/route?room=a1 will result in the following query params: {room: [a1]}? If so, it is a breaking change.

@vsavkin vsavkin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 19, 2016
@spacebeers
Copy link
Contributor Author

spacebeers commented Nov 29, 2016

@vsavkin Nope - if /something/route?room=a1&room=a2&room=a3 then the query params will end up being {room: [a1, a2, a3]} otherwise /something/route?room=a1 will be {room: a1} like normal.

vicb pushed a commit to vicb/angular that referenced this pull request Dec 9, 2016
vicb added a commit to vicb/angular that referenced this pull request Dec 9, 2016
vicb pushed a commit to vicb/angular that referenced this pull request Dec 9, 2016
vicb added a commit to vicb/angular that referenced this pull request Dec 9, 2016
vicb pushed a commit to vicb/angular that referenced this pull request Dec 9, 2016
vicb added a commit to vicb/angular that referenced this pull request Dec 9, 2016
@vicb vicb closed this in 440ef02 Dec 9, 2016
vicb added a commit that referenced this pull request Dec 9, 2016
vicb added a commit that referenced this pull request Dec 15, 2016
mprobst added a commit to mprobst/angular that referenced this pull request Mar 6, 2017
This was previously just strings, but after angular#11373 can also contain arrays of strings.

Sadly the type declaration cannot be changed for backwards compatibility.
@jcroll
Copy link

jcroll commented Mar 21, 2017

Am I crazy or does this functionality not work in reverse? If I create a Param instance and pass it to the router like so:

let queryParams = {
  'status': [
    'Deleted', 
    'Audio Issue',      
  ],
}

this.router.navigate([], { queryParams });

results in:

http://localhost:4200?status=Deleted,Audio Issue

Am I missing something? Is there any way to do this?

Edit:

I'd like it to produce this:

http://localhost:4200?status=Deleted&status=Audio Issue

@DzmitryShylovich
Copy link
Contributor

DzmitryShylovich commented Mar 21, 2017

@jcroll yeah, but it was fixed in rc5

new PR #15129

@jcroll
Copy link

jcroll commented Mar 21, 2017

I don't follow you @DzmitryShylovich, I am on ~2.4 and seeing this behavior.

Also, please see the edit to my original comment.

@n00dl3
Copy link

n00dl3 commented Mar 21, 2017

@jcroll 4.0.0-rc5

@jcroll
Copy link

jcroll commented Mar 21, 2017

Great thank you @DzmitryShylovich @n00dl3

@tschaka1904
Copy link

tschaka1904 commented Jun 28, 2017

Taking @jcroll example:

  private onQueryParamChange(): void {
    const queryParams = {};
    queryParams['status'] = this._currentStatus; //arrayOf current status('s)
    this.router.navigate([], {queryParams});
  }

I'm calling onQueryParamChange when a user selects a new filter, lets say Deleted. The URL will update to:
http://localhost:4200?status=Deleted and this._currentStatus; contains ['Deleted']
But now the user selects a second filter, lets say Audio Issue. Assuming that, the URL should update to http://localhost:4200?status=Deleted&status=Audio Issue and this._currentStatus; should contain ['Deleted', 'Audio Issue'].

Unfortunately, the URL is still pointing to http://localhost:4200?status=Deleted, but this._currentStatus; contains ['Deleted', 'Audio Issue'].

I'm observing the queryParams in ngOnInit, using:

 this.route
      .queryParams
      .subscribe(queryParams => {...});

But on it is never called, if I want to add a second value to a queryParam. I've tried to describe it already in #17609.

@tschaka1904
Copy link

Has no one some input for that?

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: router cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants