-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
path-to-regexp@^8.0.0 #117
Conversation
9517567
to
0e6408e
Compare
4357077
to
4dbd70e
Compare
I am about to rebase this onto the new CI and lint changes. I should probably just life out the changes, but I like a challenge with git and what is the point of doing this if we are not having fun while we do it? |
ed40df2
to
0d516c4
Compare
ok I think I did this correctly. There was a duplicate test and a few things which made it confusing, but I think once we get |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected]) |
9963485
to
159ed7c
Compare
@@ -400,7 +372,7 @@ describe('Router', function () { | |||
.end(cb) | |||
}, | |||
function (cb) { | |||
request(server)[method]('/zoo') | |||
request(server)[method]('/bar') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to figure out how this one worked before, but I am thinking this might be a weird git diff thing from a rebase. The /bar
route should not have "hit" it would have needed the line 2 below to flip if we kept this as zoo
. Anyway, not a really meaningful change so I am not going to try and undo what ever I missed in the merge and just push forward.
test/router.js
Outdated
.expect(shouldNotHitHandle(1)) | ||
.expect(shouldNotHitHandle(2)) | ||
.expect(shouldHitHandle(1)) | ||
.expect(shouldHitHandle(2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this change was because something was broken it just is not covered directly in these tests but is covered in the express tests. Will try and port over the correct test here.
@@ -716,27 +716,6 @@ describe('Router', function () { | |||
.expect(200, { foo: 'fizz', bar: 'buzz' }, done) | |||
}) | |||
|
|||
it('should work following a partial capture group', function (done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While looking at the express
tests I think we might be able to bring this one back in a different form. The interesting part is that the optional group is no longer captured because we dropped support for unnamed params. @blakeembrey just want to confirm this is our expected behavior.
This is the test as written in express
:
it('should work following a partial capture group', function(done){
var app = express();
var cb = after(2, done);
app.get('/user{s}/:user/:op', function(req, res){
res.end(req.params.op + 'ing ' + req.params.user + (req.url.startsWith('/users') ? ' (old)' : ''));
});
request(app)
.get('/user/tj/edit')
.expect('editing tj', cb);
request(app)
.get('/users/tj/edit')
.expect('editing tj (old)', cb);
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, there's no regex or unnamed group features right now. I think we could bring this back given time but right now it isn't possible.
Updates to
path-to-regexp@8
.So from the tests, the three breaking changes are:
router.route('/user(s?)/:user/:op')
but still have optional non-capture/user{s}/:user/:op
:name?
becomes{:name}
:name*
becomes*name
.{*name}
:name+
becomes*name
and thus equivalent to*name
so I dropped those testsI am going to link this into
express
and see locally if this set of breaking changes holds up in that test suite now.