feat: support parameter patterns#362
Conversation
|
@Kakulukian I wanted to let you know ASAP that I really appreciate the PR! It is the right direction, but unfortunately it's even more involved than this because I want to add it back in a way that ensures it still generates safe regexes. That sadly means more of a regex parser. The good news, if you're interested, is that we could start with a very simple "parser" by rejecting all meta characters except Assuming you're up for it, I'll also separately leave some review comments. |
| } | ||
|
|
||
| return value; | ||
| if (chars[i] === "(" && options?.pattern) { |
There was a problem hiding this comment.
Take a look at the previous code here: 60f2121#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80L155
I'd use that as a starting point, e.g. move the ( into the lexer itself (not part of name) and yield a PATTERN directly. Handle the fact that PATTERN is expected in a certain position in the if (param) below by doing tryConsume("PATTERN").
That's ok for me, updated with your suggestions :) |
blakeembrey
left a comment
blakeembrey
left a comment
There was a problem hiding this comment.
Feel free to omit the validation in this PR, and either of us can work on it in a follow up PR.
| while (i < chars.length && depth > 0) { | ||
| const char = chars[i]; | ||
|
|
||
| if (INVALID_PATTERN_CHARS.includes(char)) { |
There was a problem hiding this comment.
Unfortunately it’s more complex than this since you can be escaping a meta character to make it a valid character to use, so we actually need to iterate over the string and do some light parsing.
I’d also recommend leaving the parsing out of this section, as it should be part of the logic translating it into a regex as the library does accept raw tokens.
| if (token.type === "param") { | ||
| result += `(${negate(delimiter, isSafeSegmentParam ? "" : backtrack)}+)`; | ||
| if (token.pattern) { | ||
| result += `(${token.pattern})`; |
There was a problem hiding this comment.
We should do the validation of the pattern here instead. Create a new function for validation so it can be expanded over time without refactoring inside this existing code.
… escaping support, support for wildcard
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #362 +/- ##
===========================================
- Coverage 100.00% 98.17% -1.83%
===========================================
Files 1 1
Lines 668 439 -229
Branches 151 155 +4
===========================================
- Hits 668 431 -237
- Misses 0 8 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
path-to-regexp does not support this syntax at all. There was a PR to implement it, but it seems pretty dead. pillarjs/path-to-regexp#362
|
What is the status here? Would you be open to someone taking this over and moving it forward or a new PR doing something similar? The lack of this feature is blocking my ability to upgrade other libraries that depend on v8 here. I'd be happy to move this forward. I need alteration |
3 participants
I'm planning to move to the newer version of path-to-regexp. In our codebase heavily relies on regex for parameter validation, especially in some legacy components we still have running in production.
I stumbled across this comment that mentions parameter pattern support might be coming in future versions. Based on that, I went ahead and implemented the functionality in this PR.
Feel free to ignore this PR if it doesn't align with the project's direction. I understand you may have different plans for handling parameter patterns in future versions!