-
Notifications
You must be signed in to change notification settings - Fork 368
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
Better asterisk functionality #115
Conversation
I'm not sure whether this should open the gates to standalone |
😮! This is awesome, very excited to mess around with this. Potentially can delete some of my own code 😛. Regarding your last message it may be worth supporting them all. Here are some examples of my current thinking regarding // Attach middleware at an optional path without naming it.
matchPath('/my-page/?', middleware)
match('/my-page/:section?', renderSection)
// Attach middleware at an optional arbitrarily deep path without naming it.
matchPath('/my-page/+', middleware)
matchPath('/my-page/:section', renderSection)
matchPath('/my-page/:section/:subsection', renderSubSection) This also applies to hostnames // Attach middleware at an optional hostname without naming it.
matchHost('?.mydomain.com', middleware)
matchHost('www.mydomain.com', renderMainSite)
matchHost('api.mydomain.com', exposeApi)
// Attach middleware at an optional arbitrarily deep hostname without naming it.
matchHost('+.mydomain.com', middleware)
matchHost('analytics.app.mydomain.com', renderAnalytics)
matchHost('admin.mydomain.com', renderSubSection) Hopefully these examples illustrate some cases where this is useful. Curious what your thoughts are! As an additional thought, since the main purpose of these examples is to allow nameless groups without verbose syntax it might also be worth considering |
The complexity of adding it correctly is super low (see files changed, it's around three lines). I do like the |
Actually I think having looked at it a few times it is nice that ':' // Exactly one
':?' // Zero or one
':*' // Zero or more
':+' // One or more Which I think is intuitive enough. Also it helps disambiguate your previous example, which instead you would write |
One other problem I had before that I wanted to bring up was when you have a leading or trailing group. For example: '/test/(.*)' // Will not match '/test'
'(.*).test.com' // Will not match 'test.com' With the above syntax you would simply do: '/test/:*' // Will match '/test'
':*.test.com' // Will match 'test.com' Ultimately the key is that these are delimiter aware. |
@DylanPiercey Do we need a new option to handle "trailing" matches instead of prefixes. I just realised that with hostnames you'd prefer to have it do trailing groups instead of prefixed groups. E.g. |
One thing that would be nice with that syntax is that if "unnamed groups" matched. E.g. instead of being |
@blakeembrey In 1.7.0 it was possible to do Also that's what i'm talking about, I think unnamed groups could effectively achieve the same goals while also being familiar and not imposing custom implementations for these operators. Also it's not the worst thing to force a name. However programming being what it is people will probably have a hard time coming up with names and avoid them, also there is the potential for name conflicts if everything has to be named. Ultimately I would see people writing |
That shouldn't have ever worked, please feel free to double check. The path matching has always only taken into account prefixes, so it would have failed with |
I'll have to take another look at that. It may have been because of the way I was wrapping path-to-regexp. Either way this would be a welcome feature as I'm currently using it 😅 |
2bcbf82
to
ba90fa7
Compare
Updated the PR. Supporting this functionality is a one character change |
Sounds good. If you have any other concerns feel free to ping me as I'd be happy to discuss further. |
@blakeembrey any revelations? 😛 |
@dougwilson interested in your feedback also - closer to previous |
⛏️ |
Note: some maintainers prefer new tickets, some prefer keeping the discussion in the original(s). Happy to move to a new ticket if preferred. I did a lot of digging and I think this is the last notable conversation around this topic. I wanted to note that for us, we had to back down to the older 0.1.7 version of this lib because of user backlash, who expect us to support some form of the Indeed there have been a few cases I've seen where people differed on their expectations around what exactly this means (how greedy, trailing slashes, etc), but AFAICT a vast majority of the times I've seen it used, those details haven't mattered in practice thankfully. I think why our folks expect it mostly comes down to "that's what other things support." Some have seen this feature in multiple tools. These three are the most common examples I've seen referenced where they learned this behavior and came to expect it from other tools (like ours):
I haven't unfortunately yet had a chance to ask our non-technical users where they learned this expectation. My guess (without evidence) would be they learned it from copying what other people were doing rather than from previous experience. Semantically, the standalone asterisk lets the user express intent more clearly than I think The tool in question doesn't rely on express, just path-to-regexp, to be clear. I'm personally somewhat conflicted as I can see the arguments against it from a purist standpoint, but at the same time, pragmatically speaking, it seems like this ship has sailed and we're all just trying to fight it for the benefit of a minority of cases at the expense of the majority. Just adding in my experience in trying to upgrade to the latest , which of course, as always, maintainers are free to ignore since you owe us nothing Thanks! |
It makes sense to me, I believe it should act like One caveat is that it'll never quite act like express.js 4, where
|
@blakeembrey I hadn't seen URLPattern prior. Very interesting indeed!
Nothing else I had yet found, though it wasn't live for very long so there may have been others. |
Looks like there is an updated PR for this! #269 |
Related discussion in URLPattern: Named vs. Anonymous wildcard groups have inconsistent and confusing behavior |
I've created my own PR because the behavior the existing PR will introduce is not pretty. Food for thought: #270 |
/cc @DylanPiercey from #87 for review. Proper
*
behaviour that's incompatible with Express.js (in the strictest sense) but compatible with expectations.