feat: bump router to beta.1 #4752

nfantone · 2021-11-14T18:48:45Z

This is a continuation from #4321 - needed to move forward with the express@5.0 release.

Bumps router to latest beta.1 version.
Fixes and accommodates all tests.

Some remarks:

I based my changes entirely off of what's documented in pillarjs/router changelog as I'm not familiar with the internals, so unsure if it's 100% correct - but tests do pass.
Some unrelated tests were randomly failing locally for me with a socket hang up error, which seem to go away after re-running them (macOS 11.6). Is this expected behaviour?

nfantone · 2021-11-14T18:51:13Z

test/app.router.js

@@ -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){


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.

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?

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 /.

@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?

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+.

# path-to-regexp@3.2.0 > 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' } }

# path-to-regexp@0.1.7 > 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?

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.

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 router@2.0.0-beta.1? That's where the dependency to path-to-regexp@3.2.0 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.

I thought you were the one releasing router@2.0.0-beta.1? That's where the dependency to path-to-regexp@3.2.0 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.

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.

nfantone · 2021-11-14T19:06:18Z

AFAICT, build on AppVeyor is failing for node 5.12 and 6.17 due to a WARN being written to stderr. Not sure if normal, but doesn't seem to be related to the router version.

nfantone · 2021-11-17T16:37:04Z

Relevant PR blocking this one: pillarjs/router#102

dougwilson · 2021-11-17T16:41:04Z

Relevant PR blocking this one: pillarjs/router#102

How does that PR block updating to router 2.0.0-beta.1 ? The beta.1 is already released. Is there a bug in the beta.1 one release that was not outlined here that prevents using it? Or are you saying you want to close this PR and instead not upgrade to beta.1 and wait for a future router 2.x release?

nfantone · 2021-11-18T15:16:06Z

Assuming we can easily release beta.2 with the latest changes from the PR above after it's merged, yes. And not close, just update it, if that's ok.

nfantone · 2022-03-01T12:23:30Z

@dougwilson I can tell at least some of my work, present in this PR has landed, in some form, in the latest beta 😕. Would have been nice to sync on this instead of letting it die without notice.

I suppose I can close this now.

dougwilson · 2022-03-07T09:57:54Z

Hi, I'm not sure what you mean letting this die without notice. I didn't close this PR or merge anything from it yet. I was under the understanding from your comment #4752 (comment) that you were going to update this PR once beta.2 of router was released. And yes, apologies I did slip on that one a bit, but I didn't close this PR just like you requested. I'm very confused at what you mean by letting this die, though, as I thought it was waiting for beta.2 to be released and then you were going to update it. Apparently I am confused about that, I'm sorry. I can't do anything right.

feat: bump router to beta.1

cc5914b

nfantone commented Nov 14, 2021

View reviewed changes

nfantone mentioned this pull request Nov 14, 2021

Express future? #4728

Closed

aravindvnair99 requested a review from dougwilson November 14, 2021 23:48

aravindvnair99 added 5.x deps pr labels Nov 14, 2021

nfantone marked this pull request as draft November 18, 2021 15:16

nfantone mentioned this pull request Jan 3, 2022

bump v5 router to beta.1 #4321

Merged

import-brain added the needs rebase label Jan 17, 2022

dougwilson force-pushed the 5.0 branch from 04b58bd to 450c468 Compare February 5, 2022 02:47

dougwilson force-pushed the 5.0 branch from c80f806 to 318fd4b Compare February 17, 2022 05:33

nfantone closed this Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bump router to beta.1 #4752

feat: bump router to beta.1 #4752

nfantone commented Nov 14, 2021

nfantone Nov 14, 2021

dougwilson Nov 15, 2021

nfantone Nov 15, 2021

nfantone Nov 15, 2021 •

edited

Loading

nfantone Nov 15, 2021

dougwilson Nov 15, 2021

nfantone Nov 15, 2021 •

edited

Loading

dougwilson Nov 15, 2021 •

edited

Loading

nfantone Nov 15, 2021

nfantone commented Nov 14, 2021

nfantone commented Nov 17, 2021

dougwilson commented Nov 17, 2021

nfantone commented Nov 18, 2021 •

edited

Loading

nfantone commented Mar 1, 2022 •

edited

Loading

dougwilson commented Mar 7, 2022

feat: bump router to beta.1 #4752

feat: bump router to beta.1 #4752

Conversation

nfantone commented Nov 14, 2021

nfantone Nov 14, 2021

Choose a reason for hiding this comment

dougwilson Nov 15, 2021

Choose a reason for hiding this comment

nfantone Nov 15, 2021

Choose a reason for hiding this comment

nfantone Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

nfantone Nov 15, 2021

Choose a reason for hiding this comment

dougwilson Nov 15, 2021

Choose a reason for hiding this comment

nfantone Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

dougwilson Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

nfantone Nov 15, 2021

Choose a reason for hiding this comment

nfantone commented Nov 14, 2021

nfantone commented Nov 17, 2021

dougwilson commented Nov 17, 2021

nfantone commented Nov 18, 2021 • edited Loading

nfantone commented Mar 1, 2022 • edited Loading

dougwilson commented Mar 7, 2022

nfantone Nov 15, 2021 •

edited

Loading

nfantone Nov 15, 2021 •

edited

Loading

dougwilson Nov 15, 2021 •

edited

Loading

nfantone commented Nov 18, 2021 •

edited

Loading

nfantone commented Mar 1, 2022 •

edited

Loading