[#101] add query params for all REST methods #102

sewera · 2022-12-07T11:38:38Z

Resolves #101.

Brief summary of what I did:

make interfaces small and composable
fix typos in log messages and comments
I will rebuild the project once all the changes are approved

My proposed solution for this issue was to make
a couple of different interfaces describing
the capabilities of different requests.
Please find more ideas in the in-code comments.

- make interfaces small and composable - fix typos in log messages and comments

sewera

I put some additional ideas about the changes I made in the code review comments.

I will run the build task after getting an approval of the changes.

src/Client.ts

sewera · 2022-12-07T11:44:10Z

src/services/AdminService.ts

@@ -32,7 +33,7 @@ export default class AdminService extends CrudService<Admin> {
     * If the current `client.authStore.model` matches with the updated id, then
     * on success the `client.authStore.model` will be updated with the result.
     */
-    update<T = Admin>(id: string, bodyParams = {}, queryParams = {}): Promise<T> {
+    update<T = Admin>(id: string, bodyParams = {}, queryParams: BaseQueryParams = {}): Promise<T> {


Here and in other methods with queryParams: BaseQueryParams, I just assumed that all those methods will have only $autoCancel and $cancelKey, because I couldn't find anything more in the documentation.

sewera · 2022-12-07T11:45:12Z

src/services/LogService.ts

@@ -11,7 +16,7 @@ export default class LogService extends BaseService {
    /**
     * Returns paginated logged requests list.
     */
-    getRequestsList(page = 1, perPage = 30, queryParams = {}): Promise<ListResult<LogRequest>> {
+    getRequestsList(page = 1, perPage = 30, queryParams: ListLogsQueryParams = {}): Promise<ListResult<LogRequest>> {


One could argue that GetRequestsListQueryParams is a better name, but I'll leave it to the reviewer.

sewera · 2022-12-07T11:46:27Z

src/services/LogService.ts

@@ -52,10 +57,20 @@ export default class LogService extends BaseService {
    /**
     * Returns request logs statistics.
     */
-    getRequestsStats(queryParams = {}): Promise<Array<HourlyStats>> {
+    getRequestsStats(queryParams: LogStatsQueryParams = {}): Promise<Array<HourlyStats>> {


Same as in line 19, but with GetRequestStatsQueryParams.

sewera · 2022-12-07T11:51:54Z

src/services/utils/BaseService.ts

+    expand?: string;
+}
+
+export interface PaginatedQueryParams {


Paginable didn't sound right for me, but maybe it's better for consistency, I'll leave it to a reviewer's taste.

ganigeorgiev · 2022-12-07T12:03:17Z

@sewera Thank you for working on this. Sometime later this week, after the blocker in pocketbase/pocketbase#1187, I'll review it in more details and will let you know if something need to change.

ganigeorgiev · 2022-12-08T20:13:27Z

@sewera To avoid the back-and-forth, I've squash merged your changes locally + applied some minor nitpickings:

I've decided to move all query param types under services/utils/QueryParam.ts since it is easier to import this way without having to remember where the type was defined (the parameter types list also wouldn't grow significantly so it shouldn't be an issue, at least for now).
I've removed the small interfaces because it was starting to become verbose and just by reading the name (eg. PaginatedQueryParams) wasn't clear whether this type was extending the BaseQueryParams or define only a small set of fields. I've merged the parameters in the following types:
```
BaseQueryParams
ListQueryParams (base + page + sort + filter fields)
RecordQueryParams (base + expand)
RecordListQueryParams (list + expand)
LogStatsQueryParams (base + filter)
FileQueryParams (base + thumb)
```

You can find the changes in e860e34 (ignore the dist folder) and if you are not ok with them, please leave a review comment and I may reconsider it.
I'm planning a v0.8.4 release sometime this weekend.

sewera · 2022-12-08T20:34:49Z

Alright, I'll drop something in if I have anything to add by tomorrow.

sewera · 2022-12-09T08:45:47Z

@ganigeorgiev, I've written my comment, you can find it here.

sewera added 2 commits December 7, 2022 12:32

[pocketbase#101] add query params for all REST methods

0af79c3

- make interfaces small and composable - fix typos in log messages and comments

[pocketbase#101] fix imports formatting

f4ef52f

sewera commented Dec 7, 2022

View reviewed changes

sewera marked this pull request as ready for review December 7, 2022 11:54

sewera mentioned this pull request Dec 7, 2022

[Types] Make queryParams strongly-typed #101

Closed

ganigeorgiev closed this Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#101] add query params for all REST methods #102

[#101] add query params for all REST methods #102

sewera commented Dec 7, 2022

sewera left a comment

sewera Dec 7, 2022

sewera Dec 7, 2022

sewera Dec 7, 2022

sewera Dec 7, 2022

ganigeorgiev commented Dec 7, 2022

ganigeorgiev commented Dec 8, 2022 •

edited

Loading

sewera commented Dec 8, 2022

sewera commented Dec 9, 2022

[#101] add query params for all REST methods #102

[#101] add query params for all REST methods #102

Conversation

sewera commented Dec 7, 2022

sewera left a comment

Choose a reason for hiding this comment

sewera Dec 7, 2022

Choose a reason for hiding this comment

sewera Dec 7, 2022

Choose a reason for hiding this comment

sewera Dec 7, 2022

Choose a reason for hiding this comment

sewera Dec 7, 2022

Choose a reason for hiding this comment

ganigeorgiev commented Dec 7, 2022

ganigeorgiev commented Dec 8, 2022 • edited Loading

sewera commented Dec 8, 2022

sewera commented Dec 9, 2022

ganigeorgiev commented Dec 8, 2022 •

edited

Loading