Skip to content

sId, deletedAt columns addition and pagination offSet fix #74

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

m-salman-afzal
Copy link

No description provided.

Copy link

changeset-bot bot commented Nov 8, 2024

🦋 Changeset detected

Latest commit: 5dd9095

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@carbonteq/hexapp Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@m-salman-afzal m-salman-afzal requested a review from volf52 November 8, 2024 22:09
Copy link
Collaborator

@carbon-teq carbon-teq left a comment

Choose a reason for hiding this comment

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

Need to update interfaces and types with optional properties as well for this to work properly

@volf52
Copy link
Collaborator

volf52 commented Nov 11, 2024

I have shared my reasons on why I do not agree with the BaseEntity changes in the issue discussion.

The pagination offset was purposefully built in this way to be 0-index based rather than 1-index based (as is mentioned in the comments in the file). While I do recognize the utility of 1-index based calculation (which is why I added offset1 which is still part of the commented out code), it didn't make the final cut as the 0-index based calculations are simpler and more consistent with the ecosystem and with cursor based pagination systems, which means easier interop with the persistence mechanisms and the third party APIs being utilized. I mean, it is much easier to add { page + 1} to the render logic than making sure to subtracting one in the offset calculation while ensuring it doesn't go below 0. every time you need to use it. And while both can be added to hexapp in form of offset0 and offset1 (as I did in the initial versions), I eventually opted to stay with just one, consistent system rather than having two with their own explanations and an increased probability of runtime errors due to using 0-index based system while using 1-index based calculation somewhere in your code (or vice versa).

Having 10 as the default page size is probably reasonable (although very low, increasing the number of unnecessary requests). To be honest, the default options aren't really meant to be used as it is better to be explicit when creating the pagination options, ideally with both the pageSize and pageNum coming from the view layer (if the pagination options are for answering a query) or as service properties/constants (if we are using them for a third party API call). But in any case, it'll be updated to 10 or some other number lower than 100 in the coming versions.

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