Skip to content

fix(nuxt-client, nuxt): various nuxt client bugs #2054

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lrstanley
Copy link

@lrstanley lrstanley commented May 17, 2025

🚀 Changes proposed by this PR

This is a refactor of the Nuxt client, to make it more reliably support reactive (ref, computed, getter, etc) variables in the body, query params, and path params.

Security Note

I have also removed the usage of ref(), due to the potential for Cross-Request State Pollution (CRSP) when using within SSR. The usage of ref() was fairly restricted so it's unlikely a user would run into an issue in the real world, however, removing it ensures there is never a potential attack vector.

This is a breaking change. Users will need to restructure how they pass in reactive data, though it shouldn't be a large change. See the updated examples/openapi-ts-nuxt example.

Old method:

const petId = ref(1)

const result = await getPetById({
  composable: 'useFetch',
  path: {
    petId: petId,
  },
});

New method:

const petId = ref(1)

const result = await getPetById({
  composable: 'useFetch',
  path: computed(() => ({
    petId: petId.value,
  })),
});

Detailed Changes

  • Removed usage of ref() due to above security concerns.
  • headers in request options can now be reactive (config is still non-reactive).
  • query, path, and body are no longer recursively reactive, only the first level (including in the types).
    • Users who want to use reactive variables for nested data, should wrap the entire object in a computed() call, and change someRef (typed as Ref) to someRef.value (the underlying un-reactive type) when inside of computed(). Essentially, treat computed like any other Vue usecase.
  • Cleaned up various types, including documenting why some were copied from Nuxt (e.g. KeysOf).
  • Changed some utils.ts functions from directly mutating passed-in state, returning their desired state, and letting the client (and thus interceptors) decide how to merge that in.
  • Moved almost all request-modification logic into interceptors.
    • This is because, unlike most other clients, client-nuxt doesn't invoke the actual request function itself, it wraps a "request" in a Nuxt composable, and the composable decides when to invoke the request (and potentially reinvoke it when changes happen).
    • This ensures that no Ref's are made unreactive and then potentially never updated again.
  • New @hey-api/test-utils internal package. Currently holds the new msw logic for testing. It's split in 2 parts:
    • Mock server, using msw, which adds a service worker to inject a hook for all HTTP requests, forcing them to go to the mock server, rather than actually sending those requests. This means that we can do effective e2e tests without messing with the client/replacing the fetch functions, and is particularly helpful for usecases with reactivity.
      • I think this can (and probably should) be used across more of the codebase for more thorough testing. With this PR, I am fixing multiple bugs that weren't in the unit-tested functions, but how and where those functions were invoked, and I used this to find more of those same types of issues in my changes.
    • A handful of pre-defined handlers for use with that mock server.
      • Pet handlers: basic bare-bones CRUD operations for testing path, query, body, header, etc parsing.
      • Verbose handler: sends all of the data sent as part of the request, back to the client, so we can assert that the request data had what it should have.
  • Refactored tests for client-nuxt which uses that new mock server, that should also replicate/include all of the original tests.

🔗 Related bug reports/feature requests

🧰 Type of change

  • Bug fix (non-breaking change which fixes an issue).
  • Breaking change (fix or feature that causes existing functionality to not work as expected).
  • This change requires (or is) a documentation update.

📝 Notes to reviewer

Still some TODO items.

Copy link

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

Copy link

changeset-bot bot commented May 17, 2025

⚠️ No Changeset found

Latest commit: c4e72cc

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

Copy link

vercel bot commented May 17, 2025

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

Name Status Preview Comments Updated (UTC)
hey-api-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 18, 2025 4:22am

@lrstanley lrstanley changed the title fix(nuxt-client): various nuxt client bugs fix(nuxt-client, nuxt): various nuxt client bugs May 17, 2025
Copy link

codecov bot commented May 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.64%. Comparing base (7a49eca) to head (c4e72cc).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2054   +/-   ##
=======================================
  Coverage   22.64%   22.64%           
=======================================
  Files         253      253           
  Lines       21513    21513           
  Branches      818      818           
=======================================
  Hits         4872     4872           
  Misses      16635    16635           
  Partials        6        6           
Flag Coverage Δ
unittests 22.64% <ø> (ø)

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.

@lrstanley
Copy link
Author

@mrlubos, just FYI, guessing you may want to switch from pulling from raw.githubusercontent.com (that or pull authenticated), since it looks like you're hitting rate limits here:
https://github.com/hey-api/openapi-ts/actions/runs/15090824005/job/42419115612?pr=2054#step:7:896

Guessing it's related to the recent changes GH made to unauthenticated calls.

Also curious if any of the testing logic in packages/openapi-ts-tests could instead use msw instead of something like express. Then, just read the file, and serve it via a mock endpoint.

Copy link

pkg-pr-new bot commented May 18, 2025

Open in StackBlitz

@hey-api/client-axios

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-axios@2054

@hey-api/client-fetch

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-fetch@2054

@hey-api/client-next

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-next@2054

@hey-api/client-nuxt

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-nuxt@2054

@hey-api/nuxt

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2054

@hey-api/openapi-ts

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2054

@hey-api/vite-plugin

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2054

commit: c4e72cc

@lrstanley lrstanley force-pushed the fix/nuxt-ref-overhaul branch 2 times, most recently from 699c510 to 463439a Compare May 18, 2025 04:16
@mrlubos
Copy link
Member

mrlubos commented May 23, 2025

Hey, ready ready to review? I'm not seeing any issues with hitting the rate limits in CI elsewhere btw

@mrlubos
Copy link
Member

mrlubos commented May 23, 2025

@lrstanley Do you think merging this will fix the broken initial release run? It currently causes to miss publishing changelogs/releases to GitHub https://github.com/hey-api/openapi-ts/actions/runs/15206388350/job/42770418494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants