-
Notifications
You must be signed in to change notification settings - Fork 20
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
Optional groups with a leading slash do not match correctly #149
Comments
This is working as intended. For pathname patterns the path-to-regexp This is mainly done so patterns like The pattern author can opt out of this behavior by enclosing the group in an explicit grouping using curly braces. So you can write your first pattern as Does that make sense? |
It's possible to handle both scenarios, it's just that path-to-regexp doesn't.
It's great there is a way to get this working but it's definitely not intuitive to have this:
My reasoning is:
|
I think it comes down to which is the more common scenario we optimize for:
My impression was in pathnames situation (1) is more common and that is why path-to-regexp has that default. This is the first feedback I've heard to contrary. Unfortunately, I think we would need a pretty big indicator that this is a problem for many devs in order to change the API at this point. It would have been easier to consider if we had this feedback prior to launching. |
But I'm saying you can have both. You can take it from the other perspective: building URLs from a pattern:
The idea is to avoid double We have both in vue router's parser. path-to-regexp limitations should (if possible) not be a limitation for URLPattern. It doesn't look consistent to have: matchParams('/a/:p?-b', '/a-b', null) // fails because it matches
matchParams('/a/:p?-b', '/a/e-b', { p: 'e' }) // matches and pass
What do you mean by launching, I thought this was still in the feedback process as it looks like it's just Chrome so far... |
I also wanted to note I can see how the consistency here is subjective... If |
This would be a pretty big processing model change. There is no concept in the spec of looking at future characters to change the behavior of the current grouping. Also, I think it would make a somewhat magical feature and make it even more magical which I think would be bad for user understanding. It would also be a big deviation from path-to-regexp upstream. If we were to change something I think it would be more likely that we would drop the
Once we hit stable chrome we need to be careful about any breaking changes and this would definitely be a breaking change. Usage is still low, so its possible, but I also don't want to thrash the API around either. That's why I said I would need more feedback from many developers, etc, in order to justify making a breaking change. Edit: Its also implemented in deno so it would require them changing as well, etc. |
Yeah, that makes sense. I also didn't know this could change the processing model. I don't know what would be better when it comes to the prefix behavior. I personally never liked it in path-to-regexp because it created unexpected scenarios when the prefix was not
But this is still an experimental API, isn't it? And as such, it would be fine if it changes. It would be very problematic for the web platform if Chrome just pushes a new API and then forces other to adopt it. It doesn't seem to have been accepted yet.
I would say Deno is an exception as they implement new APIs to be more appealing than its competitor, node 😄 They are mainly focused on developers: as a Developer, I can choose if my code runs on Node or Deno while browsers are focused on Users and as a Developer, I cannot choose the browser my user is browsing my code with. |
Every change is a tradeoff and requires balancing different stakeholder impact. In this case I see the pro/con list of making the change as: Pro:
Con:
In general we prioritize user impact greater than web developer impact. And we prioritize web developer impact over browser engineer impact. So Con(1) is a high priority because it risks breaking sites for users. We can mitigate this by measuring usage to see if its safe to go ahead, but we can't ignore it just because its "experimental". (And usage is low, but non-zero right now.) But beyond that its not clear to me that Pro(1) outweighs Con(2). The fact that a popular pattern matching library has had this default for a long time seems significant to me. While I understand the behavior is not what you expect, it seems probable that other developers might disagree. If we hear from many other developers that this behavior is bad and/or confusing then maybe the balance starts to tip the other way. Then we can see if usage is still low enough to risk a breaking change. So I think maybe we should leave this issue open to see if other folks chime in over time. |
It's worth saying that MDN docs states the opposite about Experimental APIs : "Expect behavior to change in the future". It's also worth noting that path to regexp is not the only lib to parse URLs, it's a great source of inspiration though but it's also a bias. For instance: given a pattern of /foo/:id-bar, pattern.build() should yield /foo/-bar not /foo-bar |
@posva The reasoning here is not that it is impossible to change existing APIs. It is just not easy, and not something we can just do on a whim. Changing APIs in even minor ways can have huge impacts on users browsing the Web, as site will just break under them. The point here is that the risk of breakage here is greater than the benefit of this change.
Not per se. The way the pattern matching works today, this should yield |
@wanderview @lucacasonato @posva so... did I get this right?
|
To me that depends if the pattern in set to be strict about trailing slashes or not |
To add my own 2 cents to this issue... it is pretty clear to me that the current behavior of including the leading slash in the optional group only makes sense when there is one single variable (or wildcard) inside the path segment (and in fact, in this one particular case, it makes much more sense than not doing that). But in all other cases, objectively speaking, it makes little to no sense at all. As @posva mentioned above, you CAN have the best of both worlds by checking for this one condition. I don't see why it would matter (too much) if it causes a processing model change. I thought the main concern here was backwards compatibility. If a portion of the processing model changes, that sucks, but... you can remain backwards compatible in 99.99999% of cases while eliminating the WTFs! So isn't that a win? |
@wanderview as regards "pretty big processing model change", can you elaborate more on why that is necessarily the case? Are you saying that adding a 1 lookahead to the processing model would be unmanageably difficult? ( It seems to me this could be patched into path-to-regexp (for example) with a 2-line code tweak, simply by replacing line 178: if (prefixes.indexOf(prefix) === -1) { ... with: const suffix = ['END', 'CHAR'].includes(tokens[i].type) ? tokens[i].value : null!;
if (prefixes.indexOf(prefix) === -1 || prefixes.indexOf(suffix) === -1) { ... (Of course, you'd also have to move the Test results of my teeny tiny tweak: pathToRegexpTweaked('/a/:p?-b', undefined, {strict: true}).exec('/a-b') // === null ✅
pathToRegexpTweaked('/a/:p?-b', undefined, {strict: true}).exec('/a/-b')![1] // === undefined ✅
pathToRegexpTweaked('/a/:p?-b', undefined, {strict: true}).exec('/a/e-b')![1] // === "e" ✅
pathToRegexpTweaked('/a/:p?/b', undefined, {strict: true}).exec('/a//b') // === null ✅
pathToRegexpTweaked('/a/:p?/b', undefined, {strict: true}).exec('/a/b')![1] // === undefined ✅
pathToRegexpTweaked('/a/:p?/b', undefined, {strict: true}).exec('/a/e/b')![1] // === "e" ✅
pathToRegexpTweaked('/a/:p?', undefined, {strict: true}).exec('/a/') // === null ✅
pathToRegexpTweaked('/a/:p?', undefined, {strict: true}).exec('/a')![1] // === undefined ✅
pathToRegexpTweaked('/a/:p?', undefined, {strict: true}).exec('/a/e')![1] // === "e" ✅ Re: "I think it would make a somewhat magical feature..." As the above code snippet shows, it would be no more magical than the current implementation (in fact, it would make the existing logic roughly symmetrical.) And it is not so much a new "feature" as it is a reduction of an existing magical feature in cases where it is too magical. |
Yes, it is possible to change things to make this kind of behavior work. I wasn't trying to say its impossible. But it does seem like a big conceptual change to start altering behavior based on trailing characters after a grouping. That concept doesn't exist in the model today. And introducing it does seem like it would be more complex and magical to me. So I think there would be a risk of making things worse (more complex, harder to understand, etc) by doing something like this. On the other hand, authors can still achieve the desired effect by being explicit in their patterns. If they want your behavior for Add in that it would be a backward incompatible change and the tradeoffs point towards not making this kind of change right now. If there is a huge outpouring of developer sentiment that it needs to change, though, it would be something we could consider. |
When creating an optional group with
?
, it seems like they are not being matched correctly. Here is the test case:All
matchParams()
call should work but the first 2 ones fail.The text was updated successfully, but these errors were encountered: