feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
14.97k stars 744 forks source link

fix(client): Replace placeholders in URL with route params #3270

Closed mdartic closed 9 months ago

mdartic commented 10 months ago

Summary

Related to https://github.com/feathersjs/feathers/discussions/2962

When using the feathers client (bundled) on service with placeholders, we can't replace placeholders with route params, like it is done on server side.

This PR addresses this use case, for the rest-client.

I create some tests, not sure if I put them in the right place, neither if this will be sufficient.

Other Information

issues when running tests

When running tests on my machine, with npm test at the root directory, lerna fails:

.......Starting the MongoMemoryServer Instance failed, enable debug log for more information. Error:
        StdoutInstanceError: Instance failed to start because a library is missing or cannot be opened: "libcrypto.so.1.1"

...(after running all tests)

    ✖    1/13 targets failed, including the following:
         - @feathersjs/mongodb:test

I also see that the rest-client test suite is not runned by lerna. Don't know how to tell lerna to run rest-client tests.

I check that, when running npm test in packages/rest-client this is working

issues when running vitepress

Maybe I didn't find the doc to document feathers ;-) But when I make a fresh npm install in the docs folder, then run npm run dev, I have some issues. (maybe because it retrieves the latest version, rc.10) I can create issue for that if needed. So I write a new paragraph for the rest client, tell me if it's ok for you.

daffl commented 10 months ago

Thank you for putting this together! For the Socket client, we'd probably do a similar thing in the send method for this.path in https://github.com/feathersjs/feathers/blob/dove/packages/transport-commons/src/client.ts#L81

mdartic commented 10 months ago

Ok, I update the code in my local branch, but I'm searching where to put tests.

I think I have to put them in the transport-commons package, but do you have any advice for the file to update ?

And, by the way, do you want I push the code for the socket.io client in this branch ? Or do you prefer another PR ?

daffl commented 10 months ago

It can go in this branch and it is in the file (and line) that I linked in my previous comment.

mdartic commented 10 months ago

Sorry, I think I didn't make myself clear. That's ok for update the client.ts file, (and it's already done but not yet pushed). I was asking myself where to put tests cases. I'm enhancing the matching file client.test.ts for now :-)

mdartic commented 10 months ago

Tests added & pushed. I update the documentation too on the socket-io section. Please tell me if it's ok ? Should I update the github action workflows too to make it work ?

daffl commented 10 months ago

Great, thank you for putting this together! I'm trying to decide if we want to get it out sooner as a patch release or look at what else should go into a feature release.

mdartic commented 10 months ago

All right, If you think something could be added or is missing, don't hesitate to ask !

marshallswain commented 9 months ago

:shipit:

daffl commented 9 months ago

Will go out with the next patch release. Thank you again!