Add support for regex wildcard group #63
Conversation
There was a problem hiding this comment.
Ah, great addition! We are going to have some fun in here when we merge the upcoming express v5 changes, but this is perfect for v4 support.
EDIT: here is the WIP I was doing to integrate changes which drop a bunch of regexp related things in path-to-regexp. We are going to need to accommodate for these here, but I wanted to finish them in express first.
lib/generate-doc.js
Outdated
| const params = routeLayer.keys.map((k) => { | ||
| const params = routeLayer.keys.map((k, i) => { | ||
| const prev = routeLayer.keys[i - 1] | ||
| if (!Number.isNaN(k.name) && prev?.offset + prev?.name?.length - 2 === k.offset) { |
There was a problem hiding this comment.
I had to read this line a few times to make sure I followed, might be good to just add a comment about it for our future selves.
There was a problem hiding this comment.
I am testing this line to make sure it does not have any bugs.
There was a problem hiding this comment.
@wesleytodd I am done double checking for bugs, please re-approve and merge at your convenience.
Megapixel99
force-pushed
the
support-regex
branch
from
846b035 to
2f31d25
Compare
|
@Megapixel99 can you give me a ping when you are comfortable with this PR and I can dig in and prepare a release? |
@wesleytodd I believe this PR is good to go. If you want me to test it further (beyond the tests I wrote for it and the app I intend to use this feature in) before you merge let me know. |
|
Sorry I have been super busy trying to get express v5 wrapped up. I will loop back on this soon after we release though. Is that alright? or is this blocking you? Because I trust you and the tests, so I can push a release if you are blocked on this. |
@wesleytodd I do not not need this to be merged right away. I can use the branch for the specific project which requires this feature for now. If you need help with express v5 let me know. At one point you mentioned making me a commiter on the express project (#54 (comment)) and I am happy to contribute earlier than anticipated. |
|
Sorry, I had fallen so far behind in notifications I never saw this. You are very welcome to join us over there, we have a ton of work to do this year and a bunch of docs on how to get involved. The easiest way is probably to start with our triage team. Anyway, I think this is a good change to support v4, but I think we need a bunch of changes to support v5. I am going to start working through that problem and hopefully will land this PR and then get v5 support as well before the next release. |
Looking at Route Paths in https://expressjs.com/en/guide/routing.html, Express supports routes such as
/ab?cdand/ab*cd; however, when trying to implement/ab/cd(*)(which is mentioned in this issue), you would receive the following error:Invalid regular expression: /^(?:*)$/i: Nothing to repeat. This PR should fix that error.