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

Cursor based pagination #39

Merged
merged 46 commits into from
Sep 20, 2021
Merged

Cursor based pagination #39

merged 46 commits into from
Sep 20, 2021

Conversation

Yinon-Hever
Copy link
Contributor

No description provided.

src/fireblocks-sdk.ts Outdated Show resolved Hide resolved
@@ -5,17 +5,29 @@ import { RequestOptions } from "./types";
export class ApiClient {
constructor(private authProvider: IAuthProvider, private apiBaseUrl: string) { }

public async issueGetRequest(path: string) {
public async issueGetRequest(path: string, pageMode: boolean = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider creating a separate function for this specific logic

Copy link
Contributor Author

@Yinon-Hever Yinon-Hever Aug 19, 2021

Choose a reason for hiding this comment

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

The tradeoff here is to duplicate same code as a result of resolveWithFullResponse: true on line 17 and write GET func specific for page or edit all func's using issueGetRequest and extract body, BTW requestPromise lib deprecated, so we can fix it when we use any replacement lib.
@tomervil, what approach do u think we need to take here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But i totally agree that isn't ideal at all, just a quick win

@Yinon-Hever
Copy link
Contributor Author

@yarinvak and i spoke offline and agreed not to break the SDK, as a result of different behavior on the API vs SDK

@yarinvak yarinvak changed the title COR-1597-cursor-base-pagination cursor-base-pagination Aug 24, 2021
@tomervil tomervil changed the title cursor-base-pagination Cursor based pagination Sep 20, 2021
 into COR-1597-cursor-base-pagination

� Conflicts:
�	src/fireblocks-sdk.ts
@tomervil tomervil merged commit 4ed16dc into master Sep 20, 2021
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