-
Notifications
You must be signed in to change notification settings - Fork 12
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
Nested routes - Incompatibility with Feathers 3.x #21
Comments
Hello @oppodeldoc, I have checked the feathers documentation and the process should not be too difficult. It seems that the problem you have encountered is the only one. I will also need to check the tests as I am mocking the params object, and it is obviously different now. I am not really in favour to have a conditional for the params and then carry on legacy code. I would prefer to use npm versions e.g. version 0.x.x is compatible with feathers 2.x.x and version 1.x.x with version 3. Or maybe bump the plugin version to 3.x.x right away? Do you see any reason why we should do a conditional? Sam |
I have migrated. The change you propose is fine. You can do a PR with it and could you modify the tests? That is quite easy, there are two test to modify, the both have Sam |
Thanks @idealley! You're right about versioning and not worrying about backwards-compatibility, I just wasn't sure if you wanted to maintain it, but that's great. I'll submit a PR today! |
This commit updates the parseNestedPath function to account for new nested params in Feathers idealley#3 See idealley#21
Ok! @idealley I submitted my PR, it includes the updated tests, I didn't add a One thing I forgot was to add a note in the README about breaking changes. I'm assuming you'll bump this as a major release in versioning, might be nice to put a note in the README as well. Let me know when the latest is on NPM, and thanks again for being so responsive and helpful! |
Thank you for your help. I have merged everything. Sam |
Steps to reproduce
Hi! It looks as though Feathers 3.x introduces a breaking change to the way nested routes are handled by this plugin. I'm fairly certain it's a simple fix, but here's how to reproduce it:
/foo/:variable?something=else
instead of/foo/bar?something=else
Expected behavior
Nested paths and params should be parsed properly to avoid conflicting cache routes.
This is being caused by a change in either Feathers or Express which changes the way
params
are passed intoparseNestedPath()
here. A property ':foo' passed as a route to '/bar' used to come through like this inparams
:It now comes through nested in a 'route' object, like so:
I was able to fix this pretty easily in our app by tweaking your function so that
params.route
is now passed instead of the entireparams
object:But there are two issues here. The most important is that it will definitely break compatibility with Feathers 2. That can be remedied with a conditional, if that's the way to go. The other issue is that I'm not sure if this will mess with any other parts of the plugin. The tests pass with this code included, but I wanted to check with you @idealley before submitting a PR to see if there's a better way you can think of to do this.
Let me know, we have to fix this in our app pretty soon. I can try and just use a fork but it'd be great if you can either push this fix or merge our fix in, as above. Let me know, thanks!
The text was updated successfully, but these errors were encountered: