fix($route): correctly extract path params if path contains question mark or hash #16672
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
1 similar comment
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
gkalpak
force-pushed
the
fix-route-path-params
branch
from
96d522a to
3dc24b2
Compare
| @@ -77,5 +77,24 @@ describe('$routeParams', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should correctly extract path params containing hashes and/or question marks', function() { | |||
There was a problem hiding this comment.
Should we test for $location.path('/foo/bar?baz#qux');?
And is it intended that the "params" have no values, i.e ?baz=val?
Should we test if multiple params are correctly extracted?
There was a problem hiding this comment.
Sure. This is not the primary focus of this test, but I'm never one to shy away from adding more testcases 😛
src/ngMock/angular-mocks.js
Outdated
| @@ -2145,14 +2154,18 @@ function MockHttpExpectation(method, url, data, headers, keys) { | |||
| }; | |||
|
|
|||
| this.params = function(u) { | |||
There was a problem hiding this comment.
Should we give the params here (u, m) better names since we are already refactoring?
There was a problem hiding this comment.
What's the difference between url and u anyway?
There was a problem hiding this comment.
url is the URL of the definition (when calling $httpBackend.when(method, url)). u is the actual URL that $http was called with and we want to verify whether it matches the definition.
(Also note, url can be a string or a RegExp or a function.)
There was a problem hiding this comment.
Pretty big difference then! How about requestUrl instead of u?
There was a problem hiding this comment.
I took a different approach and renamed the MockHttpExpectation() arguments to expectedXyz (see eaf7a5db1).
Let me know what you think.
…mark or hash The `routeToRegExp()` function, introduced by 840b5f0, could not extract path params if the path contained question mark or hash. Although these characters would normally be encoded in the path, they are decoded by `$location.path()`, before being passed to the RegExp returned by `routeToRegExp()`. `routeToRegExp()` has to be able to deal with both encoded URL and decoded path, because it is being shared between `ngRoute` and `ngMocks`. This commit fixes the issue, by introducing an `isUrl` option that allows creating an appropriate RegExp for each usecase.
…estion mark or hash
…`MockHttpExpectation`
…ery/hash stripped off
gkalpak
force-pushed
the
fix-route-path-params
branch
from
fd6ff1b to
9b9e25f
Compare
…ery/hash stripped off Closes #16672
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix (and refactorings).
What is the current behavior? (You can also link to an open issue here)
The
routeToRegExp()function, introduced by 840b5f0, cannot extract path params if the path contains question mark or hash. Although these characters would normally be encoded in the path, they are decoded by$location.path(), before being passed to the RegExp returned byrouteToRegExp().routeToRegExp()has to be able to deal with both encoded URL and decoded path, because it is being shared betweenngRouteandngMocks.What is the new behavior (if this is a feature change)?
The RegExp returned by
routeToRegExp()only deals with the path part and the callers are responsible for stripping off the query and hash parts (if any).(This PR contains the code from #16670, plus some refactoring to simplify the implementation).
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Other information:
Closes #16670.