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

[now-routing-utils] Add replacing of multi-match characters #3446

Merged
merged 14 commits into from
Dec 18, 2019

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Dec 17, 2019

This makes sure to replace multi-match characters used in path-to-regexp when converting redirects.

Fixes /:path*/ being converted to /$1*/ and now converts it to /$1/

dav-is
dav-is previously requested changes Dec 17, 2019
Copy link
Contributor

@dav-is dav-is left a comment

Choose a reason for hiding this comment

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

Given this affects rewrites too, can we add tests there as well

@ijjk
Copy link
Member Author

ijjk commented Dec 17, 2019

Added tests for rewrites also

@ijjk ijjk requested review from styfle and dav-is December 17, 2019 21:15
styfle
styfle previously requested changes Dec 17, 2019
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Let's use compile() like we discussed.

compile('/:user/:users')({ user: '$1', users: '$2' })
compile('/:user*/:users')({ user: '$1', users: '$2' })
compile('/:user/:user')({ user: '$1' })

@ijjk
Copy link
Member Author

ijjk commented Dec 17, 2019

Updated to use pathToRegexp.compile 👍

@ijjk ijjk requested a review from styfle December 17, 2019 22:07
@ijjk ijjk dismissed stale reviews from dav-is and styfle December 17, 2019 22:09

changes

Co-Authored-By: Steven <steven@ceriously.com>
@ijjk ijjk requested a review from styfle December 17, 2019 22:19
@ijjk ijjk requested a review from dav-is December 17, 2019 22:31
@codecov-io
Copy link

codecov-io commented Dec 17, 2019

Codecov Report

Merging #3446 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3446   +/-   ##
=======================================
  Coverage   14.56%   14.56%           
=======================================
  Files         267      267           
  Lines       10784    10784           
  Branches     1482     1482           
=======================================
  Hits         1571     1571           
  Misses       9047     9047           
  Partials      166      166

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9687415...ac17cd2. Read the comment docs.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I think we need a few additional tests for full URLs such as

{ source: '/firebase/:id', destination: 'https://:id.firebase.com' }
{ source: '/firebase/:id', destination: 'https://:id.firebase.com:8080' }
{ source: '/firebase/:id', destination: 'https://www.firebase.com:8080?userid=:id' }

@ijjk ijjk requested a review from styfle December 17, 2019 23:30
@ijjk
Copy link
Member Author

ijjk commented Dec 17, 2019

Added more tests as we talked about

@ijjk ijjk requested a review from styfle December 18, 2019 00:19
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Amazing work 🎉

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

Successfully merging this pull request may close these issues.

4 participants