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

Fix invalid data check in query/mutation fn #1975

Merged
merged 10 commits into from
Nov 13, 2024
Merged

Conversation

HugeLetters
Copy link
Contributor

@HugeLetters HugeLetters commented Oct 28, 2024

Changes

Closes #1973

How to Review

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

@HugeLetters HugeLetters requested a review from a team as a code owner October 28, 2024 17:12
Copy link

changeset-bot bot commented Oct 28, 2024

🦋 Changeset detected

Latest commit: 204d5fd

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

This PR includes changesets to release 1 package
Name Type
openapi-react-query Patch

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

Copy link
Contributor

@kerwanp kerwanp left a comment

Choose a reason for hiding this comment

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

Thanks, could you please provide a test case as well as a patch commit?

@HugeLetters
Copy link
Contributor Author

HugeLetters commented Oct 29, 2024

After working on tests it got me thinking - should we even check if data is undefined?
Perhaps just an if(error) check would be sufficient?

Because the problem is

  • if we throw when data is undefined, we just throw undefined as error(cause error is undefined too)
  • react query already handles returning undefined as an error and will throw accordingly - perhaps it should just remain their responsibility

I would suggest doing this with a comment that we can safely return undefined since react-query itself will throw an error

if (error) {
    throw error;
}

if (data === undefined) {
    return data as never;
}

upd: its a bit complicated with mutationFn tho since undefined is actually allowed as a return value there 🫠
I guess we could return our own custom error here? we should be fine since undefined is invalid json so we cant receive it under normal circumstance

@kerwanp kerwanp added the openapi-react-query Relevant to openapi-react-query label Oct 30, 2024
@kerwanp
Copy link
Contributor

kerwanp commented Oct 30, 2024

I do not exactly remember why we checked for that data in the first place. But maybe yes we could just remove this check as we need to handle responses with no content anyway (see #1973 (comment)).

Copy link
Contributor

@kerwanp kerwanp left a comment

Choose a reason for hiding this comment

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

I'm ok with the implementation, thanks for the work. Before we merge can you just double check that we cant return undefined because of tanstack query and not because our types are wrong?

And take the opportunity to add your github in the contributors list

throw error;
}
return data;

return data as Exclude<typeof data, undefined>;
Copy link
Contributor Author

@HugeLetters HugeLetters Nov 6, 2024

Choose a reason for hiding this comment

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

shouldn't matter since the type is dictated by return type of createClient anyway, its not inferred from here - just checked that what we return is valid

Copy link
Contributor

@kerwanp kerwanp left a comment

Choose a reason for hiding this comment

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

Looks good to me! Good series of tests as well, thanks for taking the time.

@kerwanp kerwanp merged commit 621792f into openapi-ts:main Nov 13, 2024
8 checks passed
@openapi-ts-bot openapi-ts-bot mentioned this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openapi-react-query Relevant to openapi-react-query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openapi-react-query throws with null values
2 participants