Nested routes - Incompatibility with Feathers 3.x

[oppodeldoc]

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:

• Create a project with Feathers 3.x
• Set up this plugin with nested routes
• Note that routes are cached as expressions (i.e. /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 into parseNestedPath() here. A property ':foo' passed as a route to '/bar' used to come through like this in params:

params = {
    foo: 'bar' 
}

It now comes through nested in a 'route' object, like so:

params = {
    route: {
        foo: 'bar'
    }
}

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 entire params object:

function parseNestedPath(path, params) {
  const re = new RegExp(':([^\\/\\?]+)\\??', 'g');
  let match = null;

  while ((match = re.exec(path)) !== null) {
    if (Object.keys(params.route).includes(match[1])) {
      path = path.replace(match[0], params.route[match[1]]);
    }
  }

  return path;
}

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!

[idealley]

Hello @oppodeldoc, Thank you for the report. I did not migrate yet my code to 3.x.x I will do that ASAP (this afternoon or tomorrow as I have a few meetings to attend :( ).

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? I will be very happy to review your PR. Thank you for the help!

Sam

[idealley]

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 nested route in the description. I guess we could add a test in the "before" test as well... I can do it. If you are not sure let me know I will do them.

Sam

[oppodeldoc]

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!

[oppodeldoc]

Ok! @idealley I submitted my PR, it includes the updated tests, I didn't add a before test, though.

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!

[idealley]

Thank you for your help. I have merged everything.

Sam