Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngRoute): extract path params containing hashes and/or question marks correctly #16670

Closed
wants to merge 2 commits into from

Conversation

susisu
Copy link
Contributor

@susisu susisu commented Aug 20, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This PR fixes a regression introduced by 840b5f0.

What is the current behavior? (You can also link to an open issue here)
If the path ($location.path()) contains hashes and/or question marks, path params are not correctly extracted.

For example, if you set $routeProvider.when('/foo/:bar', {}) and navigate to /foo/bar%23baz, you will get $routeParams.bar === "bar", not $routeParams.bar === "bar#baz".

What is the new behavior (if this is a feature change)?
Path params are correctly extracted ($routeParams.bar === "bar#baz").

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

…arks correctly

The `routeToRegExp` function introduced by 840b5f0 could not extract path params containing hashes
and/or question marks from `$location.path()`.
Since ngMock and ngRoute deal with different subjects (encoded url for ngMock and decoded path for
ngRoute), this commit (re-)introduce separate function for each case.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@susisu
Copy link
Contributor Author

susisu commented Aug 20, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

This PR seems to unnecessarily revert a large part of 840b5f0. Can you please only make the necessary changes and leave the rest untouched.

@susisu
Copy link
Contributor Author

susisu commented Aug 21, 2018

@gkalpak Thanks for your review!
The previous parseRoute and pathRegExp functions have different types of arguments (urls and paths) so I thought they should be separate.

Anyway, I fixed it!

@gkalpak
Copy link
Member

gkalpak commented Aug 21, 2018

Awesome, thx!

The previous parseRoute and pathRegExp functions have different types of arguments (urls and paths) so I thought they should be separate.

I know, but they have almost the same logic, so it makes sense to share (see #14173 (comment)).

@gkalpak
Copy link
Member

gkalpak commented Aug 21, 2018

I created #16672 which adds some refactorings on top of your commits (mainly to simplify the implementation and decouple the two slightly different usecases of ngRoute/ngMocks).
Thx for providing the main fix, @susisu 💯

@Narretz
Copy link
Contributor

Narretz commented Aug 25, 2018

Merged as 510404e

@Narretz Narretz closed this Aug 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants