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

Query Merge #36

Merged
merged 4 commits into from
Jul 27, 2023
Merged

Query Merge #36

merged 4 commits into from
Jul 27, 2023

Conversation

aaroncox
Copy link
Member

Changes:

  • Accept maxRows on query() so it functions like the .first() call used to.
  • Modified the first() call so it's a shortcut to use query() instead of having its own logic.
  • No longer require parameters to be passed to query(), it'll return an "everything" cursor like cursor() used to.
  • Removed the cursor() call since you can just call query() to do the same thing.
  • Merged Query and QueryOptions types, and renamed to QueryParams (since the method is named Query having a type of Query was confusing).

These were all basically doing the same thing - and can now all trace back and use the query call.
@aaroncox aaroncox requested a review from dafuga July 26, 2023 05:56
@aaroncox
Copy link
Member Author

aaroncox commented Jul 26, 2023

Changes I was talking about to start merging/simplifying the API calls.

src/contract/table.ts Outdated Show resolved Hide resolved
})
}

if (params.json) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so it won't objectify data if json isn't true? Should we make that the default? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, the data will come back using core name types.

This isn't the json parameter passed into get_table_rows, this is kinda a new parameter that is either:

  • json: false (the default) the call will return core typed data
  • json: true converts all the core typed data to a plain JSON object before returning it to the app.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should rename this, to avoid confusion with the parameter you'd use for get_table_rows, even though it kinda does the same thing 🤔

By default, every call to get_table_rows I think will have json: false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh ok that makes sense, I got confused because Serializer.objectify(row) doesn't do what I thought it did 😅 I think that the json param is fine here, but I guess that one other option is to have a serialize option that can take the "json" value (and potentially other values in the future??).

Copy link
Member Author

@aaroncox aaroncox Jul 27, 2023

Choose a reason for hiding this comment

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

I'm not sure what that'd do based on your comment, but it's definitely something we can explore! 👍

And yeah - the Serializer.objectify I think literally just calls JSON.parse(JSON.stringify()) on something to convert all the typed data (Name, Asset, etc) and makes it plain ol' JSON with strings and numbers.

Since everything's always typed everywhere on our end, I figured I'd be nice to have an option and say "give me non-typed data".

Co-authored-by: Daniel Fugere <[email protected]>
return new TableCursor<RowType>({
abi: this.abi,
client: this.client,
first(maxRows: number, params: QueryParams = {}): TableCursor<RowType> {
Copy link
Contributor

@dafuga dafuga Jul 26, 2023

Choose a reason for hiding this comment

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

Should we just get rid of this .first() method like we talked about? I'm not sure it adds much value at this point since we can get the same with .query({maxRows})

Copy link
Member Author

Choose a reason for hiding this comment

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

We can definitely - I wasn't sure if this still is useful as shorthand for that query though. No strong opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah same here lol. I guess we can keep it around for now.

Copy link
Contributor

@dafuga dafuga left a comment

Choose a reason for hiding this comment

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

Looking good! 👍 I left you some comments, but in any case I think we can merge this!

@aaroncox aaroncox merged commit 3a0ced2 into dev Jul 27, 2023
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.

2 participants