Closed pshrmn closed 7 years ago
This is intentional and I don't fully understand the issue. Can you go more into the issue instead of the differences in the RegExp? If you're using end: false
, why wouldn't you mount under /parent
directly, then use /
and /child
with end: true
? Doing that would have the same effect of what you're asking for and the same effect as 1.x, which was stripping the trailing /
in /parent/
for you.
Sorry, perhaps I went a bit overboard with the tables but my point was that I cannot match parent/child/
without also matching parent
.
Maybe my issue is that I just don't see the purpose of strict
in this new setup. It seems to me that its only use is to say that a path cannot end with a trailing slash. If strict
is true
and you want the segment to end with a delimiter, then the value of end
doesn't seem to matter.
Anyways, if you don't see this as a problem, I'll understand if you close the issue. I was hoping to use v2, but I can revert to v1.7.
If you’d like to share the exact behaviour you’d like to see, I can more closely figure out if we can make it work how you want and what the trade-offs are. As it is, and from what I understand, this is intentional. Strict works the same as before - it enforces the trailing character as written. The main change in 1.x and 2.x was removing the auto-trimming of trailing characters, which I believe is the right behaviour. 2.x is now looking for the end or a trailing segment, while 1.x had special behaviour.
I have this routes setup:
{
path: 'packages/',
children: [{
path: ':packageID/'
}]
}
Normally I would not use the trailing slashes, but this site is hosted on GitHub pages, so I'm generating index.html
files for all of my pages. This setup is not typical/ideal, but I think that the use case is still valid for people who just want to enforce trailing slashes even if they have dynamic server routing.
On the server, I end up with this structure:
root/
├── index.html
└── packages/
├── index.html
├── vue
│ └── index.html
└── react
└── index.html
To state the obvious, if the user navigates to example.com/packages/
they will receive packages/index.html
and if they navigate to example.com/packages/vue/
, they will receive packages/vue/index.html
. I would consider this a parent/child relationship between packages/
and packages/vue/
. However, there doesn't appear to be a way to match packages/
while allowing for extra segments. This is the crux of the issue for me. I can match packages/vue
with packages
, but I cannot match packages/vue/
with packages/
.
My current "fix" is to just use packages
as the path. That matches packages/
and packages/vue/
as expected, but also matches packages
, which I would prefer it not to.
I use path-to-regexp
's compile
function to generate pathnames, and so my internal links end up as <a href="/packages">
. Clicking on those within the application will update the current location's pathname to /packages
, but if I open up the link in another tab, the location's pathname will be /packages/
because the browser fixes the URI. I don't love having two paths represent the same resource, which is why I would prefer to be able to enforce the trailing slash.
As I said before, I understand if you think this is outside of the scope of what path-to-regexp
should handle. It would certainly come in handy for the project, but it might add unnecessary complexity to how the regular expressions are built. I'm not even sure what I think the correct regular expression would look like here. I mentioned /packages\//
before, but if you wanted to enforce the trailing slash on children, it would probably be something more along the lines of /packages\/(?:$|(?:.+\/$))/
(where you force the trailing delimiter at the end of the path).
Awesome, that example helps a ton. I’m on vacation now, but I’ll revisit and see what I can do when I’m back. I’d love to properly support this use case and I thought I had a flag for being “suffix only”, but can’t find it now on mobile. Adding back 1.x behaviour around if the trailing input character is a delimiter should be easy enough to do and I’ll just need to think through the side effects of the change.
I'm not sure my example falls in this issue.
Currently with the following express
code:
const app = require('express')()
app.use('/', function(req, res, next) {
console.log('/')
next()
})
app.use('/a', function(req, res, next) {
console.log('/a')
next()
})
app.use('/a/', function(req, res, next) {
console.log('/a/')
next()
})
app.use('/a/b', function(req, res, next) {
console.log('/a/b')
next()
})
app.listen(3000)
calling /a/b
the server prints:
/
/a
/a/
/a/b
In the v2 instead /
and /a/
middlewares don't match that path.
Is this a feature?
@allevo Yes. #116 gets into some more detail about it, but the gist of it is that you should think of a path that ends with a delimiter as a request for the index resource of that directory. This issue is more about matching pathnames when all segments end with trailing slashes (e.g. allow /a/
to match for requests for /a/
and /a/b/
but not /a
or /a/b
).
@pshrmn Sorry, I had a vacation and have fallen behind my open source projects. I thought about this a bit though, and believe the correct approach for this issue to support when the trailing character is part of delimiters
and end: false
, we can make the final character a match requirement without a lookahead.
That sounds good to me. I had been thinking that a flag might be necessary, but automatic detection would definitely be better.
Also, I hope you don't feel that it is really necessary to apologize about any delay. Taking some time away from OSS sounds like a good way to prevent burning out. Plus, apologizing also makes me feel worse about just waiting for you and not trying to fix this and opening a PR myself :sweat_smile:.
@pshrmn I have a PR coming that works with your example, but I just wanted to quickly check - this functionality used to rely specifically on strict: true
right? I can't see anyway this would have worked without it in past versions.
I believe that it was { strict: true, end: false }
.
I am a little bit confused looking at the tests. Isn't this the same as v1 behavior?
['/test/route', ['/test/']]
With the new matching, I thought that it was supposed to be:
['/test/route', null],
['/test/route/', ['/test/']]
because /test/route
is a sibling of /test/
.
I'm not sure why you expect it to work like your second example. Maybe there's something miscommunicated, because I implemented it according to your requirements above. The regexp will never check the character at the end of the string for a non-ending match - it's unnecessary work and I'm not sure what advantage doing so would give. From a non-ending point of view, the only thing that matter is the beginning you asked to match.
Perhaps you're misunderstanding the test suite? Can you tell me the path input, match input and match output you're expecting to see? E.g. with /test/
and { end: false }
it matches both /test/route
and /test/route/
because the beginning matches.
The new code looks fine to me; I guess I'm still wrapping my head around the trailing delimiter changes.
I thought that with v2 /
and /thing
are sibling routes, which is why /
doesn't match /thing
. If we prepend a segment before each to get /root/
and /root/thing
, then I would think that /root/
wouldn't match /root/thing
. That is why I thought that /test/
should match /test/route/
but not /test/route
.
There’s just no way to make it match based on a trailing slash at the end of the input match. I’m still not sure what difference the final character on the input string makes to the path though, it doesn’t care about things that come later in the string, just that the beginning matches.
Perhaps there was a different feature you wanted here, such as “suffix based” matching? I could still aim to add that, but the current change would work for you - there’s no way to ensure the specific implementation you want exactly since the path matching is always left to right. Whether the child matches or not would depend on the path of your child, and in initial use case this passes (it would strip packages/
to leave x/
).
The sibling cases should still be passing the same with this change. The change only affects non-ending mode when you end with a delimiter (e.g. /
). It did however require a change to the “sibling” case in non ending mode, since before it checked the next character was a delimiter - now it just ensures it’s ending like you wrote it (when what you wrote ends in a delimiter).
I’m not sure this makes 100% sense, so if it doesn’t let me know. Maybe I can make some time to have a call - it’s probably way easier to talk it over and figure it out.
I think I have a better grasp on this now. Thanks for adding this!
Related to #116 and #120, but not the exact same issue.
In
1.7.0
, you could create a path with a trailing slash,strict: true
, andend: false
and receive a regular expression that would matchparent/
andparent/child/
(orparent/child
) but notparent
. This was useful for static file style paths whereparent/
representsparent/index.html
andparent/child/
representsparent/child/index.html
.The above does not seem possible in v2. I understand that this is because trailing delimiters are now explicitly handled, but I feel like there should be a way to enforce a trailing delimiter while also allowing for additional segments. Perhaps a flag could be used for this? The biggest problem I can see is that it might be difficult to allow
parent/
andparent/child/
but notparent/child
(since it is a sibling ofparent/
).parent
false
parent/
true
parent/child/
true
parent
true
true
/^parent\/$
false
parent/
true
parent/child/
false
parent
true
false
/^parent\/(?:\/(?=$))?$/i
false
parent/
true
parent/child/
false
parent
false
true
/^parent\/(?=\/\|$)/i
false
parent/
true
parent/child/
false
parent
false
false
/^parent\/(?:\/(?=$))?(?=\/\|$)/i
false
parent/
true
parent/child/
false
The regular expressions returned by the current
master
branch are slightly different, but the results are the same.Currently I can use the path without a trailing delimiter and rely on the browser to fix that for me, but I would prefer to write my paths to enforce that there is a trailing slash. I'm only really running into this issue because I have a bit of a funky setup, but I think that it could also be annoying for Django users (where all URIs end with a trailing slash by default).