idealley / feathers-hooks-rediscache

Set of caching hooks and routes for feathersjs.
MIT License
38 stars 12 forks source link

Improve path parsing to include optional params on dynamic routes #18

Closed oppodeldoc closed 6 years ago

oppodeldoc commented 6 years ago

Summary

Other Information

This is a simple change to allow nested routes such as author/:authorId?/book to be handled properly. The question mark in these routes allows optional params in express and so the RegEx had to be tweaked to account for it. It also fixes a tiny typo in the README. :)

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 74.884% when pulling ed31dbc2eb86ee2c7b17274c7686f6f72242f232 on oppodeldoc:dynamic-routes-params into 2be327b08d2023eb73e5f2f9d54a0e70d290f215 on idealley:master.

oppodeldoc commented 6 years ago

So to expand upon this just a bit, we're using Express routing options to build a few fun things into our Feathers routes. Routing in Express is pretty RegEx-y: https://expressjs.com/en/guide/routing.html

We're using a very common pattern of a route/:likeThis? so the question mark was breaking the path parser. The PR tweaks the matching in the path parser and supports this pattern, though clearly in the future it might be cool to match other patterns, if needed. Thanks again, I'm hoping you can get this merged in and on NPM soon. Cheers!

idealley commented 6 years ago

Thank you for all the work and for the interest. I was going to bed and saw your PR. I check this first things in the morning, and merge it. It is true that I did not yet have to deal with those optional params.

Just to let you know we use this "plugin" in production thus we continue to improve it and care about it ;) Thank you again for improving it!

oppodeldoc commented 6 years ago

Thanks @idealley it can definitely wait until morning, thanks in advance!