openapi-react-query

[kerwanp]

Changes

This Pull-request brings a new package openapi-tanstack-query that combines the power of openapi-fetch and @tanstack/react-query together.

Here is a simple example:

import createClient from 'openapi-fetch';
import createQueryClient from 'openapi-tanstack-query';

const client = createClient<paths>();
const $api = createQueryClient(client);

const Component = () => {
    const { data, error, isLoading } = $api.useQuery('get', '/users');

    if (isLoading || !data) return "Loading...";

    return data.map((user) => <div key={user.id}>{user.name}</div>);
}

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?
WIP

Checklist

• Handles useQuery
• Handles useMutation
• Handles useSuspenseQuery
• Unit tests
• docs/ updated (if necessary)

[kerwanp]

Hey @drwpow ! 👋

As I heavily use Openapi Apis in React and React Native projects I needed a library that improve the experience with @tanstack/react-query.

Do you think that the package has its place in this repository, or do you prefer it to be external?

[drwpow]

I’m a big fan of TanStack Query, and I think it pairs really well with openapi-fetch! And at initial glance I appreciate all the thought and attention you’ve put into this!

When it comes to adding it to this repo, I’d be open to it if you’d take the lead in maintaining it! Much of the work in OSS happens after packages get published with bugfixes and maintenance (and in this case keeping up with TanStack Query). And as I have my hands full with other work, this new package wouldn’t get the bugfixes, maintenance, and updates it needs from me.

So if this sounds good and you’d like to be a maintainer on this project, you’ll get full access including being able to update the other packages like you did with openapi-fetch. But you can work on the other packages as much or as little as you like (and if you are just interested in the TanStack wrapper, that’s fine!). And with this new package, you can own it yourself and get full credit; we’ll just configure CI to add this package if you’re open to it.

[kerwanp]

That is not a problem, I would be glad to join the project as a contributor!

As I work a lot with openapi daily on different projects, I have a real need for the openapi-typescript ecosystem. So it does make sense to me to contribute to it.

As this repository is already well known, I think it is better to have the library here. I'm not here for the credit, but to have a library that will make my life much easier.

And I'm completely open to contribute to other packages

[drwpow]

That is not a problem, I would be glad to join the project as a contributor!

Awesome! Invited you to the project 🙂

[drwpow]

Since this is for an unreleased package and isn’t blocking anything, feel free to mark as Ready for review when you’re happy with it. Then we can discuss what you’d like to do before cutting the initial version / getting this connected to changesets & publishing in CI automatically.

[kerwanp]

Hey! @drwpow I made a big mistake by omitting the remote when pushing and all my commits went into the main branch of this repo.

I instant cancelled the CI and did a reset with a force push to avoid mess in the git history.

It would be great if you could disable bypass so I can't do this mistake again. Really sorry for this..

---

I think that the current features are good for a first v0.0.1, once published I'll start adding the library to some projects I'm currently working on so I can try it in real case scenarios.

I also wrote documentation. As I'm not a great writer, it might need to be rewritten.

[drwpow]

Hey! @drwpow I made a big mistake by omitting the remote when pushing and all my commits went into the main branch of this repo.

I instant cancelled the CI and did a reset with a force push to avoid mess in the git history

Oh no worries; that shouldn’t be possible and needs to be fixed. I recently switched to the new “Rules” system which is the newer version of branch protections. I want maintainers to be able to self-merge PRs if needed but not push to main. Not a huge deal! We’ll fix it.

I think that the current features are good for a first v0.0.1, once published I'll start adding the library to some projects I'm currently working on so I can try it in real case scenarios.

I also wrote documentation. As I'm not a great writer, it might need to be rewritten.

Awesome! And I’m not a good writer either so you’re in good company 😅.

I’ll give this a review either today or tomorrow. Thanks again for adding this! This will be great.

Also if I could ask: if there was anything you felt could be improved from the CONTRIBUTING docs in each package (anything you wish you would have known), always welcome submissions on those as well if you think of any 🙂

[kerwanp]

I’ll give this a review either today or tomorrow. Thanks again for adding this! This will be great.

Great! Don't be scared to nitpick this PR so future ones can be more smooth.

Also if I could ask: if there was anything you felt could be improved from the CONTRIBUTING docs in each package (anything you wish you would have known), always welcome submissions on those as well if you think of any 🙂

The first thing I would do is making a root CONTRIBUTING. I realized too late that they exist.

And the second thing is adding commit convention. As the scope of changes become wider, following a convention like https://www.conventionalcommits.org/en/v1.0.0/ would give a better git history and would make committing easier. As currently I don't know if I should put the package name in the commit or not, uppercase or not, imperative or not, etc.
It could be something simple like feat(openapi-fetch): add ability to run this.

And the last thing would be to add git hooks, to ensure that test, build, lint and format are all good before pushing but this is more about QOL.

When I have a bit of time I'll open an issue with my different recommendations and ideas so we have a dedicated place to discuss those. I have way less experience in open-source than you (almost none) but I'll do my best to bring my knowledge to this project.

[drwpow]

Ah figured out what’s needed: I didn’t realize this was an option (this is fairly new):

Tried pushing to main and it rejects as anticipated:

We still need some overrides to self-approve and self-merge if-needed (because technically merging a PR counts as a commit). In the future, I’d love for all PRs to need approval! But we’ll need to just have more people regularly available before that’s possible (at this stage, maintenance & updates are more preferable than procedure).

[drwpow]

This looks great, and IMO is ready to merge! The comments I left are all minor, and nonblocking. Happy to merge this when you’re ready!

Just a note: this should be configured for changesets to do its thing on every merge to main. How it works is: if any package.json contains a version that doesn’t exist on npm, it’ll publish on merge to main. That’s why the [ci] deploy PRs exist: to double-check and review versions that will get published on merge. But it’s important to note that the PR is optional; any bump to package.json versions will publish a new version (though that’s highly-discouraged, because changes should accompany changeset versions)

[drwpow]

The first thing I would do is making a root CONTRIBUTING. I realized too late that they exist.

Done! Good idea.

And the second thing is adding commit convention. As the scope of changes become wider, following a convention like https://www.conventionalcommits.org/en/v1.0.0/ would give a better git history and would make committing easier. As currently I don't know if I should put the package name in the commit or not, uppercase or not, imperative or not, etc. It could be something simple like feat(openapi-fetch): add ability to run this.

Ah that’s a good callout. This library doesn’t follow commit guidelines because of a differing philosophy from something like semantic-release (which a buddy of mine maintains). The semantic-release philosophy is that commit messages contain important information that describe changes that can then be used to compile changelogs.

I personally subscribe more to the philosophy that while commit messages can contain important information about what’s changed, I see commit message information as distinct from changelog information which can have additional layers of fidelity on what changed, and why. And realize that changelog information may happen over several commits/PRs and don’t have to be 1:1.

There’s no right or wrong, but that’s why this library uses Changesets to generate its changelogs separately from commit messages. While it has problems (parts of it aren’t maintained well), and I’d be open to another solution, I do like the overall philosophy of separating repo-relevant information (commit messages) from user-relevant information (changelogs). So all that said, while this repo could enforce stricter guidelines, it would only be purely aesthetic and the opinion of the code maintainers (of which I don’t feel strongly enough to do so, but I’m open to you implementing it if you think it helps the project quality!).

Lastly, anecdotally, I just see fewer and fewer projects enforcing this level of strictness, so it just seemed to be “falling out of fashion,” but again, I am not against it (and have practiced it in the past).

And the last thing would be to add git hooks, to ensure that test, build, lint and format are all good before pushing but this is more about QOL.

Another good suggestion! I personally hate git hooks and disable them 😆 but when they run quickly I don’t mind them (and Biome is about as quick as it gets if we want to run linting/formatting as a hook). I do feel strongly that build/test should never be hooks as those can be too longrunning to be done effectively. But linting/formatting may actually be a QoL improvement with as fast/minimal as they are.

Though we could also have some idea of “opt-in hooks” where you can run a command to set up slower build/test hooks if a user so-chooses, while still having those off by default. Open to that as well!

[kerwanp]

Thanks for your review! I will do the changes tonight or tomorrow.

Ah that’s a good callout. This library doesn’t follow commit guidelines because of a differing philosophy from something like semantic-release (which a buddy of mine maintains). The semantic-release philosophy is that commit messages contain important information that describe changes that can then be used to compile changelogs.

I totally agree that generating changelogs based on commit messages is not the best idea as you can easily have a bloated changelog with too specific details that does not descrive the real changes for the end user of the library.

I usually use semantic commits to analyze technical debt on my customers' projects. This way it is easy to calculate the number of LOC pushed for fixes, features, etc. But I don't think it matters here. Anyway, I think it is important to add in the CONTRIBUTING a part for commit messages, even if it is "Write whatever you want" so no one get scared about their git history before creating their PR.

Another good suggestion! I personally hate git hooks and disable them 😆 but when they run quickly I don’t mind them (and Biome is about as quick as it gets if we want to run linting/formatting as a hook). I do feel strongly that build/test should never be hooks as those can be too longrunning to be done effectively. But linting/formatting may actually be a QoL improvement with as fast/minimal as they are.

I totally understand your sentiment about Git hooks! I cannot count the number of times I used --no-verify to skip minutes of tests after doing an innocent change. A good in-between could be adding Lint staged to only format and check the staged files (and not the whole codebase). It drastically reduce the time and it automatically add the changes to your commit so you don't have to: git commit, "Oh no...", pnpm lint, pnpm format, git commit. This way it brings QOL without being a pain for contributors.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 46911de

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

[kerwanp]

I guess we have to remove the parallel build as openapi-react-query will start building when openapi-fetch build hasn't finished.

In the future it would be interesting to use turbo or nx. The setup should be straight forward as the repo is still fairly small

[drwpow]

I guess we have to remove the parallel build as openapi-react-query will start building when openapi-fetch build hasn't finished.

In the future it would be interesting to use turbo or nx. The setup should be straight forward as the repo is still fairly small

pnpm run executes in the correct order! It may just need to be marked as a devDependency or something. If the dependencies are configured correctly, there are no race conditions (it actually does a lot more than npm run in this regard—more than most people realize!).

[kerwanp]

Hey! I went touching some grass those last days but I finally got a bit of time to resolve the last discussions of this review.

We should be good!

[drwpow]

This looks great! This is more than good enough to merge as-is.

This should be all configured and ready to go with changesets, too, so any subsequent PRs with changesets should open release PRs.