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

Align readMany and readByQuery in SDK #11187

Closed
3 tasks done
br41nslug opened this issue Jan 20, 2022 · 6 comments · Fixed by #11204
Closed
3 tasks done

Align readMany and readByQuery in SDK #11187

br41nslug opened this issue Jan 20, 2022 · 6 comments · Fixed by #11204

Comments

@br41nslug
Copy link
Member

br41nslug commented Jan 20, 2022

Preflight Checklist

Describe the Bug

The current javascript sdk documentation still refers to the readByQuery function on the ItemsService which judging from the code was merged into the readMany function.

@rijkvanzanten
Copy link
Member

The real issue here is that the SDK isn't aligned with the internal services from Directus itself at the moment. We should split up readMany and readByQuery in the JS SDK as well, so it aligns with the ItemService's readMany and readByQuery calls

@rijkvanzanten rijkvanzanten changed the title JS SDK items.readByQuery documentation Align readMany and readByQuery in SDK Jan 20, 2022
@br41nslug
Copy link
Member Author

I fully agree that thats the underlying issue. but the immediate issue here is that the docs and code are out of sync.

@rijkvanzanten
Copy link
Member

@br41nslug Wanna shoot a quick PR to those docs? 🙂

@br41nslug
Copy link
Member Author

Damnit i hate writing docs 🤣 but sure i'll give it a go

@joselcvarela
Copy link
Member

The real issue here is that the SDK isn't aligned with the internal services from Directus itself at the moment. We should split up readMany and readByQuery in the JS SDK as well, so it aligns with the ItemService's readMany and readByQuery calls

I am not sure if this is the right approach.
SDK is just a thin wrapper around the REST API. Although ItemsService has a readMany which can read multiple items by primary keys, the same does not happen on the REST API, because we can only pass a single primary key on GET.
If we would implement readMany on SDK, it was simply a filter: { [primaryKey]: { _in: [keys] } } on current readMany, so I am not sure if makes sense to create such method 🤔

@rijkvanzanten
Copy link
Member

If we would implement readMany on SDK, it was simply a filter: { [primaryKey]: { _in: [keys] } } on current readMany, so I am not sure if makes sense to create such method

To me, that actually makes sense. If you have people that are writing both extensions, and use the SDK on their own projects, this consistent naming system with different type signatures is very confusing!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants