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

[BUGFIX beta] Update route-recognizer to v0.2.0. #13768

Merged
merged 1 commit into from
Jun 28, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 28, 2016

This update includes a number of fixes regarding encoding/decoding from tildeio/route-recognizer#91.

I have included a snippet from the upstream fixing PR below, but the full PR description itself contains a wonderful explanation of the problem and solution.

This addresses the following:

  • Adds many tests for non-standard routes and paths (with percent-encodings and/or unicode characters)
  • Adds normalization for added routes and recognized paths to improve the matching of non-standard routes and paths
  • Fixes a bug where dynamic segments are not percent-encoded during generation
  • Fixes a bug where dynamic segments are not percent-decoded during recognition
  • Adds tests (and some code) to ensure that star segments in routes do not get decoded during recognition
  • Adds a flag that can be turned off by consumers to opt-out of the changes introduced here

It may only affect users who have routes with:

  • percent-encoded or unicode characters
  • dynamic segments that may match paths with percent-encoded characters
  • dynamic segments that are generated using values that have characters that should be encoded (e.g. forward slash: /)

It may cause breaking changes for users who have workarounds in place to encode/decode before generation/recognition. The breakage can be fixed by removing the workaround.

It will likely not cause breaking changes for users who have static routes with percent-encoded or unicode characters. All static route/path combinations that currently match will also match after this PR. Static routes that used to be considered distinct (e.g. /f%20o and /f o) are now normalized to the same thing. Users with affected static route/path combos likely need to do nothing; they may simply end up with superfluous routes that can be removed.

Full diff from v0.1.10 to v0.2.0: tildeio/route-recognizer@v0.1.10...v0.2.0

/cc @bantic

This update includes a number of fixes regarding encoding/decoding.

Snippet from the upstream fixing PR:

This addresses the following:

  * Adds many tests for non-standard routes and paths (with percent-encodings and/or unicode characters)
  * Adds normalization for added routes and recognized paths to improve the matching of non-standard routes and paths
  * Fixes a bug where dynamic segments are not percent-encoded during generation
  * Fixes a bug where dynamic segments are not percent-decoded during recognition
  * Adds tests (and some code) to ensure that star segments in routes do *not* get decoded during recognition
  * Adds a flag that can be turned off by consumers to opt-out of the changes introduced here

It may only affect users who have routes with:

  * percent-encoded or unicode characters
  * dynamic segments that may match paths with percent-encoded characters
  * dynamic segments that are generated using values that have characters that should be encoded (e.g. forward slash: `/`)

It **may cause breaking changes** for users who have workarounds in place to encode/decode before generation/recognition. The breakage can be fixed by removing the workaround.

It will likely **not cause breaking changes** for users who have static routes with percent-encoded or unicode characters. All static route/path combinations that currently match will *also* match after this PR. Static routes that used to be considered distinct (e.g. `/f%20o` and `/f o`) are now normalized to the same thing. Users with affected static route/path combos likely need to do nothing; they may simply end up with superfluous routes that can be removed.
@rwjblue rwjblue merged commit 8892fa2 into emberjs:master Jun 28, 2016
@rwjblue rwjblue deleted the update-route-recognizer branch June 28, 2016 19:33
@nathanhammond
Copy link
Member

I'd like to revert this change to delay it from landing in 2.7. I don't believe that this is the correct solution anymore. The correct solution is to do segment-at-a-time matching which addresses approximately all of this in a far-simpler manner without requiring nearly as much code and no API surface area changes.

/cc @bantic

@krisselden
Copy link
Contributor

@nathanhammond why revert? If you can pass the tests with simpler solution, great but this is just the equivalent of making case insensitive look-up by storing lowercase form and lower casing the search term on search.

What was the API change?

@rwjblue
Copy link
Member Author

rwjblue commented Jul 18, 2016

What was the API change?

The API change was a route-recognizer internal one (it now looks for a Normalizer where it didn't previously), however this is not an API exposed by Ember (certainly not publicly). If future refactors to route-recognizer choose not to use a Normalizer concept so be it (as long as its version bumps appropriately).

If you can pass the tests with simpler solution, great but this is just the equivalent of making case insensitive look-up by storing lowercase form and lower casing the search term on search.

Agreed. We should not revert.

@bantic
Copy link
Member

bantic commented Jul 18, 2016

The shape of the handlers also changed slightly, which might be what is being referred to here. They used to be objects like {handler, names} and are now objects like {handlers, names, shouldDecodes} (the change was introduced here: tildeio/route-recognizer@0adc94e#diff-3cd8d4dfb826ed2ebdceb9c43e729d8eR371). The reason for adding the shouldDecodes property is to ensure that Star (aka glob) Segments do not get decoded during route recognition.

@nathanhammond
Copy link
Member

Yeah, I'm okay with this sticking around, had a conversation in #dev-ember about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants