Skip to content
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

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

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

mdartic
Copy link
Contributor

@mdartic mdartic commented Sep 4, 2023

Summary

Related to #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
Copy link
Member

daffl commented Sep 5, 2023

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
Copy link
Contributor Author

mdartic commented Sep 5, 2023

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
Copy link
Member

daffl commented Sep 5, 2023

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

@mdartic
Copy link
Contributor Author

mdartic commented Sep 5, 2023

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 :-)

docs(client): Add documentation for route placeholders
@mdartic
Copy link
Contributor Author

mdartic commented Sep 5, 2023

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
Copy link
Member

daffl commented Sep 9, 2023

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
Copy link
Contributor Author

mdartic commented Sep 10, 2023

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

@daffl daffl changed the title feat(client): Replace placeholders in URL with route params fix(client): Replace placeholders in URL with route params Oct 11, 2023
Copy link
Member

@marshallswain marshallswain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@marshallswain
Copy link
Member

:shipit:

@daffl daffl merged commit a0624eb into feathersjs:dove Oct 11, 2023
2 checks passed
@daffl
Copy link
Member

daffl commented Oct 11, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants