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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

blakeembrey
Copy link
Member

/cc @DylanPiercey from #87 for review. Proper * behaviour that's incompatible with Express.js (in the strictest sense) but compatible with expectations.

@coveralls
Copy link

coveralls commented Aug 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2bcbf82 on asterisk-functionality into 144ff8c on master.

@blakeembrey
Copy link
Member Author

I'm not sure whether this should open the gates to standalone /? and /+ though. I'm also not sure if we shouldn't just promote /:path* instead.

@DylanPiercey
Copy link

DylanPiercey commented Aug 23, 2017

😮! 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 /? and /+ use cases:

// 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.
However my personal opinion is that, although I think it aligns with my expectations, this should only be supported if it's possible without adding a ton of complexity to path-to-regexp.

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 /:*, /:?, and /:+ instead. Although I personally think they are a bit ugly.

@blakeembrey
Copy link
Member Author

The complexity of adding it correctly is super low (see files changed, it's around three lines). I do like the /:? as it's less ambiguous. My main concern was path matching with something like /test? - it could be confused as a query parameter. At first glance it seems like something like /my-page/? may be saying optional /my-page.

@DylanPiercey
Copy link

DylanPiercey commented Aug 23, 2017

Actually I think having looked at it a few times it is nice that /:? removes ambiguity. I think it could make things simpler as well, essentially:

':' // 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 /:?(test). What are your thoughts on that? I am just bouncing ideas around with the hopes of achieving the most flexible/sensible implementation.

@DylanPiercey
Copy link

DylanPiercey commented Aug 23, 2017

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.
Is it ugly? I'm not sure 😛

@blakeembrey
Copy link
Member Author

@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. :subdomain?.example.com isn't possible today because it considers prefixes, not suffixes.

@blakeembrey
Copy link
Member Author

One thing that would be nice with that syntax is that if "unnamed groups" matched. E.g. instead of being (.*) is was :(.*). I'll think on it though. What do you think is the main advantage of the shortened syntax is? Is adding a name really so bad (it can be a form of documentation - e.g. :subdomain.example.com)?

@DylanPiercey
Copy link

DylanPiercey commented Aug 23, 2017

@blakeembrey In 1.7.0 it was possible to do *.test.com I believe.

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 :subdomain.host.com and /test/:path so often that they may end up asking for the shorthands anyway. I could easily be wrong though.

@blakeembrey
Copy link
Member Author

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 test.com but not with x.test.com. I'm proposing a feature so you can flip it and it'll use suffixes (e.g. compile(':path*.test.com')() would properly output test.com instead of .test.com).

@DylanPiercey
Copy link

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 😅

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling ba90fa7 on asterisk-functionality into 144ff8c on master.

@blakeembrey
Copy link
Member Author

Updated the PR. Supporting this functionality is a one character change \w+ to \w*. I'll leave it open for a bit to decide.

@DylanPiercey
Copy link

Sounds good. If you have any other concerns feel free to ping me as I'd be happy to discuss further.

@DylanPiercey
Copy link

@blakeembrey any revelations? 😛

@blakeembrey
Copy link
Member Author

@dougwilson interested in your feedback also - closer to previous * as :* (e.g. unnamed and without custom group). Not sure personally how I feel about such a simple delimiter meaning so much, so additional feedback is 👍

@mightyiam
Copy link

⛏️

@jayphelps
Copy link

jayphelps commented Mar 16, 2021

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 /* wildcard behavior without param names or (.*) regex.

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):

  • Express v4 and earlier
  • Cloudflare routes and page rules
  • react-router

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 :foo or :foo* or similar, since in our case they are expressing they are not interested in the actual value of the path that is matched. IMO this isn't the strongest argument in the world, though, but it does have some legs when there isn't a non-technical word that accurately describes the path in question (splat, rest, etc are technical terms).

I think * vs (.*) is in fact less error prone and less intimidating for a non-technical person, as the other characters (especially the period) has no meaning to them but an asterisk has a non-technical meaning and usage.

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 :shipit:

Thanks!

@blakeembrey
Copy link
Member Author

It makes sense to me, I believe it should act like (.*) if it is to be added back. It's pretty likely it'll be re-introduced due to discussions in https://github.com/WICG/urlpattern, for compatibility with existing URL patterns. However, there's a few other breaking changes that the discussions in urlpattern may introduce so I've been waiting to see whether I should do them all in one go or keep it separate.

One caveat is that it'll never quite act like express.js 4, where * expanded in some odd spots. It'll be pretty close though. Are you aware of any other issues hindering upgrading? The list of other breaking changes for URLPattern compatibility are:

  1. Case sensitivity (currently insensitive by default)
  2. Strict trailing slash (currently non-strict by default)
  3. Magic prefixes (i.e. /:foo? automatically making the / optional)

@jayphelps
Copy link

@blakeembrey I hadn't seen URLPattern prior. Very interesting indeed!

Are you aware of any other issues hindering upgrading

Nothing else I had yet found, though it wasn't live for very long so there may have been others.

@HansBrende
Copy link

Looks like there is an updated PR for this! #269

@HansBrende
Copy link

@HansBrende
Copy link

I've created my own PR because the behavior the existing PR will introduce is not pretty. Food for thought: #270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants