fix(router): fix URL serialization so special characters are only enc… #22337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vicb
reviewed
Feb 21, 2018
packages/router/src/url_tree.ts
Outdated
| @@ -336,32 +336,41 @@ function serializeSegment(segment: UrlSegmentGroup, root: boolean): string { | |||
| * sub-delims = "!" / "$" / "&" / "'" / "(" / ")" | |||
| * / "*" / "+" / "," / ";" / "=" | |||
| */ | |||
| export function encode(s: string): string { | |||
| return encodeURIComponent(s) | |||
| export function encodeUriParams(s: string, type: 'query' | 'segment' = 'query'): string { | |||
vicb
reviewed
Feb 21, 2018
packages/router/src/url_tree.ts
Outdated
| .replace(/%5B/gi, '[') | ||
| .replace(/%5D/gi, ']'); | ||
|
|
||
| encoded = type === 'segment' ? |
There was a problem hiding this comment.
could we have a common internal function with the replace up there and 2 exported functions for query and segment with more docs ?
|
You can preview dc0de4a at https://pr22337-dc0de4a.ngbuilds.io/. |
|
You can preview b5d4bdf at https://pr22337-b5d4bdf.ngbuilds.io/. |
|
You can preview 0e008dd at https://pr22337-0e008dd.ngbuilds.io/. |
|
You can preview 5431d29 at https://pr22337-5431d29.ngbuilds.io/. |
vicb
reviewed
Feb 21, 2018
packages/router/src/url_tree.ts
Outdated
| .replace(/%2C/gi, ',') | ||
| .replace(/%3B/gi, ';'); | ||
| function encodeUriString(s: string, type: 'query' | 'segment' = 'query'): string { | ||
| let encoded = encodeURIComponent(s) |
vicb
reviewed
Feb 21, 2018
packages/router/src/url_tree.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Encodes a string for use in the query string or fragment. This function should be used to |
There was a problem hiding this comment.
could we be more explicit:
Used to encode:
- URL fragments,
- parameter keys,
- parameter values.
vicb
reviewed
Feb 21, 2018
packages/router/src/url_tree.ts
Outdated
| */ | ||
| export function encodeUriSegment(s: string): string { | ||
| return encodeUriString(s) | ||
| .replace(/\(/g, '%28') |
There was a problem hiding this comment.
add a note on why we have to do it (in the header comment or inline)
The name tells me that this is used to encode a segment (the part between 2 "/") while the comment tell me it encodes the path ?
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
You can preview 6c6af62 at https://pr22337-6c6af62.ngbuilds.io/. |
|
You can preview 36319d2 at https://pr22337-36319d2.ngbuilds.io/. |
|
You can preview 7dae85c at https://pr22337-7dae85c.ngbuilds.io/. |
IgorMinar
approved these changes
Feb 22, 2018
IgorMinar
added
action: cleanup
|
I realized that I don't see in this diff is two more changes we discussed:
I see this PR being marked as "breaking change" but that's not the case as far as I can tell. As we discussed, all the previously serialized urls will be correctly parsed and recognized (so bookmarked urls will keep on working). And urls that were previously incorrectly serialized will now work correctly - that's not a breaking change. |
jasonaden
force-pushed
the
fix_url_seralization
branch
from
7dae85c
to
0751b03
Compare
jasonaden
removed
the
action: cleanup
This was referenced Mar 15, 2018
leo6104
pushed a commit
to leo6104/angular
that referenced
this pull request
Mar 25, 2018
…oded where needed (angular#22337) Fixes: angular#10280 This change brings Angular largely in line with how AngularJS previously serialized URLs. This is based on [RFC 3986](http://tools.ietf.org/html/rfc3986) and resolves issues such as the above angular#10280 where URLs could be parsed, re-serialized, then parsed again producing a different result on the second parsing. Adjustments to be aware of in this commit: * Query strings will now serialize with decoded slash (`/`) and question mark (`?`) * URI fragments will now serialize the same as query strings, but hash sign (`#`) will also appear decoded * In the URI path or segments (portion prior to query string and/or fragment), the plus sign (`+`) and ampersand (`&`) will appear decoded * In the URL path or segments, parentheses values (`(` and `)`) will now appear percent encoded as `%28` and `%29` respectively * In the URL path or segments, semicolons will be encoded in their percent encoding `%3B` NOTE: Parentheses and semicolons denoting auxillary routes or matrix params will still appear in their decoded form -- only parentheses and semicolons used as values in a segment or key/value pair for matrix params will be encoded. While these changes are not considered breaking because applications should be decoding URLs and key/value pairs, it is possible that some unit tests will break if comparing hard-coded URLs in tests since that hard coded string will represent the old encoding. Therefore we are releasing this fix in the upcoming Angular v6 rather than adding it to a patch for v5. PR Close angular#22337
leo6104
pushed a commit
to leo6104/angular
that referenced
this pull request
Mar 25, 2018
…only encoded where needed (angular#22337)" This reverts commit 094666d.
leo6104
pushed a commit
to leo6104/angular
that referenced
this pull request
Mar 25, 2018
…oded where needed (angular#22337) This change brings Angular largely in line with how AngularJS previously serialized URLs. This is based on RFC 3986 and resolves issues such as the above angular#10280 where URLs could be parsed, re-serialized, then parsed again producing a different result on the second parsing. Adjustments to be aware of in this commit: * URI fragments will now serialize the same as query strings * In the URI path or segments (portion prior to query string and/or fragment), the plus sign (`+`) and ampersand (`&`) will appear decoded * In the URL path or segments, parentheses values (`(` and `)`) will now appear percent encoded as `%28` and `%29` respectively * In the URL path or segments, semicolons will be encoded in their percent encoding `%3B` NOTE: Parentheses and semicolons denoting auxillary routes or matrix params will still appear in their decoded form -- only parentheses and semicolons used as values in a segment or key/value pair for matrix params will be encoded. While these changes are not considered breaking because applications should be decoding URLs and key/value pairs, it is possible that some unit tests will break if comparing hard-coded URLs in tests since that hard coded string will represent the old encoding. Therefore we are releasing this fix in the upcoming Angular v6 rather than adding it to a patch for v5. Fixes: angular#10280 PR Close angular#22337
leo6104
pushed a commit
to leo6104/angular
that referenced
this pull request
Mar 25, 2018
jmleoni
pushed a commit
to jmleoni/angular
that referenced
this pull request
Oct 6, 2018
…oded where needed (angular#22337) Fixes: angular#10280 This change brings Angular largely in line with how AngularJS previously serialized URLs. This is based on [RFC 3986](http://tools.ietf.org/html/rfc3986) and resolves issues such as the above angular#10280 where URLs could be parsed, re-serialized, then parsed again producing a different result on the second parsing. Adjustments to be aware of in this commit: * Query strings will now serialize with decoded slash (`/`) and question mark (`?`) * URI fragments will now serialize the same as query strings, but hash sign (`#`) will also appear decoded * In the URI path or segments (portion prior to query string and/or fragment), the plus sign (`+`) and ampersand (`&`) will appear decoded * In the URL path or segments, parentheses values (`(` and `)`) will now appear percent encoded as `%28` and `%29` respectively * In the URL path or segments, semicolons will be encoded in their percent encoding `%3B` NOTE: Parentheses and semicolons denoting auxillary routes or matrix params will still appear in their decoded form -- only parentheses and semicolons used as values in a segment or key/value pair for matrix params will be encoded. While these changes are not considered breaking because applications should be decoding URLs and key/value pairs, it is possible that some unit tests will break if comparing hard-coded URLs in tests since that hard coded string will represent the old encoding. Therefore we are releasing this fix in the upcoming Angular v6 rather than adding it to a patch for v5. PR Close angular#22337
jmleoni
pushed a commit
to jmleoni/angular
that referenced
this pull request
Oct 6, 2018
…only encoded where needed (angular#22337)" This reverts commit 094666d.
jmleoni
pushed a commit
to jmleoni/angular
that referenced
this pull request
Oct 6, 2018
…oded where needed (angular#22337) This change brings Angular largely in line with how AngularJS previously serialized URLs. This is based on RFC 3986 and resolves issues such as the above angular#10280 where URLs could be parsed, re-serialized, then parsed again producing a different result on the second parsing. Adjustments to be aware of in this commit: * URI fragments will now serialize the same as query strings * In the URI path or segments (portion prior to query string and/or fragment), the plus sign (`+`) and ampersand (`&`) will appear decoded * In the URL path or segments, parentheses values (`(` and `)`) will now appear percent encoded as `%28` and `%29` respectively * In the URL path or segments, semicolons will be encoded in their percent encoding `%3B` NOTE: Parentheses and semicolons denoting auxillary routes or matrix params will still appear in their decoded form -- only parentheses and semicolons used as values in a segment or key/value pair for matrix params will be encoded. While these changes are not considered breaking because applications should be decoding URLs and key/value pairs, it is possible that some unit tests will break if comparing hard-coded URLs in tests since that hard coded string will represent the old encoding. Therefore we are releasing this fix in the upcoming Angular v6 rather than adding it to a patch for v5. Fixes: angular#10280 PR Close angular#22337
jmleoni
pushed a commit
to jmleoni/angular
that referenced
this pull request
Oct 6, 2018
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…oded where needed
Fixes: #10280
This PR might cause breaking changes. While it shouldn't generally cause any issues, if an application were relying on the over-aggressive encoding Angular previously used, this PR is less aggressive leaving more of the URL intact.