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.

Sign up for GitHub

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

feat: bump router to beta.1 #4752

Closed
wants to merge 1 commit into from
+22 −21
Closed

feat: bump router to beta.1 #4752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/downloads/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ app.get('/', function(req, res){

// /files/* is accessed via req.params[0]
// but here we name it :file
app.get('/files/:file(*)', function(req, res, next){
app.get('/files/:file(.*)', function(req, res, next) {
var filePath = path.join(__dirname, 'files', req.params.file);

res.download(filePath, function (err) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
"proxy-addr": "~2.0.5",
"qs": "6.7.0",
"range-parser": "~1.2.1",
"router": "2.0.0-alpha.1",
"router": "2.0.0-beta.1",
"safe-buffer": "5.1.2",
"send": "0.17.1",
"serve-static": "1.14.1",
Expand Down
2 changes: 1 addition & 1 deletion test/Router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ describe('Router', function(){
throw new Error('should not be called')
}

router.all('*', function (req, res) {
router.all('(.*)', function (req, res) {
res.end()
})

Expand Down
37 changes: 19 additions & 18 deletions test/app.router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ describe('app.router', function(){
var app = express();
var router = new express.Router({ mergeParams: true });

router.get('/*.*', function(req, res){
router.get('/(.*)\.(.*)', function(req, res){
var keys = Object.keys(req.params).sort();
res.send(keys.map(function(k){ return [k, req.params[k]] }));
});
Expand All @@ -302,7 +302,7 @@ describe('app.router', function(){
var app = express();
var router = new express.Router({ mergeParams: true });

router.get('/*', function(req, res){
router.get('/(.*)', function(req, res){
var keys = Object.keys(req.params).sort();
res.send(keys.map(function(k){ return [k, req.params[k]] }));
});
Expand Down Expand Up @@ -527,7 +527,7 @@ describe('app.router', function(){
it('should allow escaped regexp', function(done){
var app = express();

app.get('/user/\\d+', function(req, res){
app.get('/user/(\\d+)', function(req, res){
res.end('woot');
});

Expand Down Expand Up @@ -560,7 +560,7 @@ describe('app.router', function(){
it('should capture everything', function (done) {
var app = express()

app.get('*', function (req, res) {
app.get('(.*)', function (req, res) {
res.end(req.params[0])
})

Expand All @@ -572,7 +572,7 @@ describe('app.router', function(){
it('should decode the capture', function (done) {
var app = express()

app.get('*', function (req, res) {
app.get('(.*)', function (req, res) {
res.end(req.params[0])
})

Expand All @@ -584,7 +584,7 @@ describe('app.router', function(){
it('should denote a greedy capture group', function(done){
var app = express();

app.get('/user/*.json', function(req, res){
app.get('/user/(.*).json', function(req, res){
res.end(req.params[0]);
});

Expand All @@ -596,7 +596,7 @@ describe('app.router', function(){
it('should work with several', function(done){
var app = express();

app.get('/api/*.*', function(req, res){
app.get('/api/(.*)\.(.*)', function(req, res){
var resource = req.params[0]
, format = req.params[1];
res.end(resource + ' as ' + format);
Expand All @@ -610,7 +610,7 @@ describe('app.router', function(){
it('should work cross-segment', function(done){
var app = express();

app.get('/api*', function(req, res){
app.get('/api(.*)', function(req, res){
res.send(req.params[0]);
});

Expand All @@ -626,7 +626,7 @@ describe('app.router', function(){
it('should allow naming', function(done){
var app = express();

app.get('/api/:resource(*)', function(req, res){
app.get('/api/:resource(.*)', function(req, res){
var resource = req.params.resource;
res.end(resource);
});
Expand All @@ -648,7 +648,8 @@ describe('app.router', function(){
.expect('122', done);
})

it('should eat everything after /', function(done){
// TODO: I don't think this is valid anymore?
xit('should eat everything after /', function(done){
Copy link
Author

Choose a reason for hiding this comment

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

I temporarily disabled this test because I was't sure this was supported in router-beta.1. If someone could confirm it is, indeed, not, I'll go ahead and remove it. If it is, please let me know how to update the syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good question. I'm not sure myself and would need to look to find out. Do you want to do that leg work, or are you asking to defer that to someone else?

Copy link
Author

Choose a reason for hiding this comment

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

I was hoping someone else could answer off the top of their heads. That' ok - I'll try to take a look myself. What happens now is that, even after updating the syntax, the :user* group still matches everything after /.

Copy link
Author

@nfantone nfantone Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@dougwilson Any reason why router would use path-to-regexp 3.2.0? Was that the latest at the time it was released? It's now on 6.2.0. Would it make sense to target that or would it be too big a leap?

Copy link
Author

Choose a reason for hiding this comment

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

So, based on this test from pillarjs/router which addresses the same situation, I can confirm the above doesn't work anymore with path-to-regexp 3.x+.

# [email protected]

> const p = require('path-to-regexp')
> p.match('/u/:users*')('/u/nfantone/foo/bar')
{
  path: '/u/nfantone/foo/bar',
  index: 0,
  params: { users: [ 'nfantone', 'foo', 'bar' ] }
}
> p.match('/u/:users(.*)')('/u/nfantone/foo/bar')
{
  path: '/u/nfantone/foo/bar',
  index: 0,
  params: { users: 'nfantone/foo/bar' }
}
# [email protected]

> const p = require('path-to-regexp')
> '/u/nfantone/foo/bar'.match(p('/u/:users*'))
['/u/nfantone/foo/bar', 'nfantone', '/foo/bar']

What shall we do with the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens now is that, even after updating the syntax, the :user* group still matches everything after /.

Gotcha. So the test section this is in is *, and I see you changed everything to (.*) instead. Does that change not work for this instance? Also should the name of this test section be updated?

Any reason why router would use path-to-regexp 3.2.0? Was that the latest at the time it was released? It's now on 6.2.0. Would it make sense to target that or would it be too big a leap?

As nuch as I can remember, this is just what someone helped make a pull request for. It would probably be a question for that author of the PR. If you believe you can make it even more up to date, a PR would be wecome in that regard.

So, based on this test from pillarjs/router which addresses the same situation, I can confirm the above doesn't work anymore with path-to-regexp 3.x+.

Ah, I see. We should probably also add thoses tests here too.

Copy link
Author

@nfantone nfantone Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So the test section this is in is , and I see you changed everything to (.) instead. Does that change not work for this instance?

Unfortunately, no. As shown in the snippet above it matches all segments until the end of the path (e.g.: { users: 'nfantone/foo/bar' }, instead of just nfantone.

As much as I can remember, this is just what someone helped make a pull request for.

I thought you were the one releasing [email protected]? That's where the dependency to [email protected] comes from.

If you believe you can make it even more up to date, a PR would be wlecome in that regard.

I could try and see if router works with newer versions of path-to-regexp. Seems like it shouldn't be too hard to pull off.

Copy link
Contributor

@dougwilson dougwilson Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought you were the one releasing [email protected]? That's where the dependency to [email protected] comes from. I think you might be confusing it with some other related change.

Sure, I make releases, but a release contains commits, many of which are pull requests that I did not author. I did not author the upgrade of path-to-regexp, you even linked above to the commit where github shows at the top the commit author. I actually did check :) What other change would I be confusing it with, can you elaborate?

This is the pull request: pillarjs/router#42

I could try and see if router works with newer versions of path-to-regexp. Seems like it shouldn't be too hard to pull off.

Awesome, thanks! Do you think we should finish this first to get a release out and then do that, or delay this until that is done? I know you were originally wondering why express 5 keeps getting delayed, and I can say from experience that it is these things that pop up that make it take a while :) I am for just trying to get this out first and then that a look at that upgrade so we can get 5 out the door sooner rather than later.

Copy link
Author

Choose a reason for hiding this comment

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

What other change would I be confusing it with, can you elaborate?

Got confused myself, apologies. I even deleted that bit - but your answer got here first.

Do you think we should finish this first to get a release out and then do that, or delay this until that is done?

Done already 👉🏼 pillarjs/router#102. Turns out, it just worked out-of-the-box.

var app = express();

app.get('/user/:user*', function(req, res){
Expand All @@ -663,7 +664,7 @@ describe('app.router', function(){
it('should span multiple segments', function(done){
var app = express();

app.get('/file/*', function(req, res){
app.get('/file/(.*)', function(req, res){
res.end(req.params[0]);
});

Expand All @@ -675,7 +676,7 @@ describe('app.router', function(){
it('should be optional', function(done){
var app = express();

app.get('/file/*', function(req, res){
app.get('/file/(.*)', function(req, res){
res.end(req.params[0]);
});

Expand All @@ -687,7 +688,7 @@ describe('app.router', function(){
it('should require a preceding /', function(done){
var app = express();

app.get('/file/*', function(req, res){
app.get('/file/(.*)', function(req, res){
res.end(req.params[0]);
});

Expand All @@ -699,7 +700,7 @@ describe('app.router', function(){
it('should keep correct parameter indexes', function(done){
var app = express();

app.get('/*/user/:id', function (req, res) {
app.get('/(.*)/user/:id', function (req, res) {
res.send(req.params);
});

Expand All @@ -711,7 +712,7 @@ describe('app.router', function(){
it('should work within arrays', function(done){
var app = express();

app.get(['/user/:id', '/foo/*', '/:bar'], function (req, res) {
app.get(['/user/:id', '/foo/(.*)', '/:bar'], function (req, res) {
res.send(req.params.bar);
});

Expand Down Expand Up @@ -762,7 +763,7 @@ describe('app.router', function(){
var app = express();
var cb = after(2, done);

app.get('/user(s)?/:user/:op', function(req, res){
app.get('/user(s?)/:user/:op', function(req, res){
res.end(req.params.op + 'ing ' + req.params.user + (req.params[0] ? ' (old)' : ''));
});

Expand Down Expand Up @@ -1170,7 +1171,7 @@ describe('app.router', function(){
var app = express();
var path = [];

app.get('*', function(req, res, next){
app.get('(.*)', function(req, res, next){
path.push(0);
next();
});
Expand All @@ -1190,7 +1191,7 @@ describe('app.router', function(){
next();
});

app.get('*', function(req, res, next){
app.get('(.*)', function(req, res, next){
path.push(4);
next();
});
Expand Down