Skip to content

Conversation

brolnickij
Copy link
Contributor

@brolnickij brolnickij commented Sep 9, 2025

  • implement a defineQueryOptions factory for every xxxQuery
  • normalized all parts of each query key using _JSONValue (so keys are consistent and type-safe)
  • updated the docs for xxxQuery - kept only the "canonical usage" examples
  • updated examples in openapi-ts-pinia-colada
  • simplify generated query keys by removing body / headers

Close #2597

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Sep 9, 2025

🦋 Changeset detected

Latest commit: 3351637

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

This PR includes changesets to release 1 package
Name Type
@hey-api/openapi-ts 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

vercel bot commented Sep 9, 2025

@brolnickij is attempting to deploy a commit to the Hey API Team on Vercel.

A member of the Team first needs to authorize it.

@brolnickij brolnickij changed the title WIP refactor(pinia-colada): migrate queries to defineQueryOptions refactor(pinia-colada): migrate queries to defineQueryOptions Sep 9, 2025
@mrlubos
Copy link
Member

mrlubos commented Sep 9, 2025

@brolnickij hold your horses on this one a bit, #2582 is almost ready and it contains some new concepts

@brolnickij
Copy link
Contributor Author

@mrlubos

i didnt see your note and already implemented the core logic

totally fine if #2582 breaks this - i update my work and help with the migration once it lands!

Copy link

codecov bot commented Sep 9, 2025

Codecov Report

❌ Patch coverage is 5.71429% with 66 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.85%. Comparing base (91bc2d0) to head (3351637).

Files with missing lines Patch % Lines
...enapi-ts/src/plugins/@pinia/colada/queryOptions.ts 6.12% 46 Missing ⚠️
...s/openapi-ts/src/plugins/@pinia/colada/queryKey.ts 4.76% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2610      +/-   ##
==========================================
+ Coverage   23.81%   23.85%   +0.04%     
==========================================
  Files         379      379              
  Lines       36945    36887      -58     
  Branches     1623     1623              
==========================================
+ Hits         8798     8799       +1     
+ Misses      28134    28075      -59     
  Partials       13       13              
Flag Coverage Δ
unittests 23.85% <5.71%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

pkg-pr-new bot commented Sep 9, 2025

Open in StackBlitz

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/codegen-core@2610
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2610
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2610
npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2610

commit: 3351637

@mrlubos
Copy link
Member

mrlubos commented Sep 9, 2025

@brolnickij merged my big thing, seems there are no conflicts and you just need to re-run the snapshots

@mrlubos
Copy link
Member

mrlubos commented Sep 10, 2025

@brolnickij how did you find working with the latest on main? Any feedback on the Symbol API?

@brolnickij
Copy link
Contributor Author

how did you find working with the latest on main? Any feedback on the Symbol API?

i didnt dive super deep, but its a solid upgrade!

selectors over hard‑coded imports + idempotent generation = big win, i like it

@mrlubos
Copy link
Member

mrlubos commented Sep 10, 2025

how did you find working with the latest on main? Any feedback on the Symbol API?

i didnt dive super deep, but its a solid upgrade!

selectors over hard‑coded imports + idempotent generation = big win, i like it

thanks! I was just curious since it's not even documented yet

@brolnickij
Copy link
Contributor Author

thanks! I was just curious since it's not even documented yet

AI (used the right way) make life a bit easier xD

@brolnickij brolnickij requested a review from mrlubos September 11, 2025 16:10
@brolnickij
Copy link
Contributor Author

brolnickij commented Sep 11, 2025

@mrlubos

  • my PR feat(defineQueryOptions): add overloaded for optional params posva/pinia-colada#386 was approved, which let me restore the behavior with optional options
  • i updated examples in openapi-ts-pinia-colada, they now follow current @pinia/colada best practices
  • after a deep internal review, the @pinia/colada plugin now works reliably across different scenarios, including the old behavior (no breaking changes)
  • simplifies generated query keys by removing body / headers

P.S. issue #2619 is a complex, multi-layered problem, so it needs to be addressed in its own dedicated PR

@mrlubos
Copy link
Member

mrlubos commented Sep 11, 2025

Thanks! Why did you remove the usage of context in queries? Do we not want to propagate that signal so it cancels fetch functions?

@brolnickij
Copy link
Contributor Author

Why did you remove the usage of context in queries? Do we not want to propagate that signal so it cancels fetch functions?

oh, i removed it because i saw issues when using @hey-api/client-nuxt together with the @pinia/colada plugin

i need to think about how to handle this so that, despite the @hey-api/client-nuxt issues, we can still use client-nuxt with the @pinia/colada plugin after the update

@brolnickij
Copy link
Contributor Author

@mrlubos done!

query: async (context) => {
const { data } = await BarBazService.getFooBar({
...options,
...context,
throwOnError: true
});
return data;
}

@brolnickij
Copy link
Contributor Author

brolnickij commented Sep 11, 2025

right now @pinia/colada plugin is not compatible with @hey-api/client-nuxt (it actually wasnt compatible before either)

to fix the compatibility issues, we should open a separate PR based on #2619

@brolnickij
Copy link
Contributor Author

brolnickij commented Sep 11, 2025

after looking deeper into how client-nuxt works, im increasingly thinking we shouldnt mix the behavior of client-nuxt and @pinia/colada

we might even want to disallow using them together

i post an update in #2619 once im confident in my analysis

#2618 (comment)

@mrlubos
Copy link
Member

mrlubos commented Sep 11, 2025

Interesting. I'd think we can fix the Nuxt client to remove whatever issues you've found?

@brolnickij
Copy link
Contributor Author

I'd think we can fix the Nuxt client to remove whatever issues you've found?

we need to look into it and see how complex it would be, but thats a separate discussion..

@mrlubos
Copy link
Member

mrlubos commented Sep 12, 2025

@brolnickij happy with this state of pull request or any more changes coming?

@brolnickij
Copy link
Contributor Author

@mrlubos im done, ready for review!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@pinia/colada: use defineQueryOptions() for reactive options
3 participants