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

Continue adding fetch methods to the SDK #1060

Merged

Conversation

juangm
Copy link
Collaborator

@juangm juangm commented Jan 21, 2025

No description provided.

@juangm juangm requested a review from cesarenaldi January 21, 2025 21:04
@juangm juangm self-assigned this Jan 21, 2025
Copy link

height bot commented Jan 21, 2025

This pull request has been linked to 1 task:

💡Tip: Add "Close T-23563" to the pull request title or description, to a commit message, or in a comment to mark this task as "Done" when the pull request is merged.

Copy link

vercel bot commented Jan 21, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
lens-sdk-example-web-wagmi ⬜️ Ignored (Inspect) Jan 23, 2025 9:59am

Copy link

changeset-bot bot commented Jan 22, 2025

⚠️ No Changeset found

Latest commit: 445741e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

* ```
*/
export function useAccountManagers(
args: AccountManagersArgs,
Copy link
Member

Choose a reason for hiding this comment

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

I didn't check this in detail, but wherever the request is optional, and from the example useAccountManagers() it seems so, this param should optional.

Otherwise this will only work with an awkward empty object useAccountManagers({}) if no specific args are needed.

export function useAccountManagers({
suspense = false,
...request
}: AccountManagersArgs & { suspense?: boolean }): SuspendableResult<Paginated<AccountManager>> {
Copy link
Member

Choose a reason for hiding this comment

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

I believe in the case of optional args for non-suspense signature we need to also provide a default value here:

Suggested change
}: AccountManagersArgs & { suspense?: boolean }): SuspendableResult<Paginated<AccountManager>> {
}: AccountManagersArgs & { suspense?: boolean } = {}): SuspendableResult<Paginated<AccountManager>> {

This is not visible from the outside, it's just needed to TS to support all signatures.

@juangm juangm merged commit fcc938a into next Jan 23, 2025
3 checks passed
@juangm juangm deleted the juan/T-23563-react-add-second-batch-of-missing-methods-for-fetch branch January 23, 2025 10:25
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