-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
feat: add support for cookies auth #1850
Conversation
|
🦋 Changeset detectedLatest commit: fe43b88 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
4cff0e3
to
db4cd59
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1850 +/- ##
==========================================
- Coverage 57.24% 57.23% -0.01%
==========================================
Files 191 191
Lines 25952 25954 +2
Branches 1964 1964
==========================================
Hits 14856 14856
- Misses 11087 11089 +2
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@hey-api/client-axios
@hey-api/client-fetch
@hey-api/client-next
@hey-api/nuxt
@hey-api/client-nuxt
@hey-api/openapi-ts
@hey-api/vite-plugin
commit: |
db4cd59
to
dc80d9e
Compare
Fixed up the header truthiness check, and also added a cookie security scheme to the 3.0.x & 3.1.x test specs; hopefully that will fix the code coverage warning. |
Strange, the built/minified code is differnt on one particular version of the macOS tests. Is that something we need to be concerned about? Coverage also failed with a similar issue. Any thoughts? |
Hm, so this doesn't actually work, at least for cross-origin requests! Even if you include I suppose this will work if people are using this for same-origin requests, or to generate clients for use with nodejs, at least. |
An option would be to pass For me, I could have my origin (the host serving the website) directly accept and forward API requests to my API backend. The default is |
@kelnos hi, to clarify, is the failing CI pipeline the only thing left to resolve or are you looking for any input from me? |
Ah, hm, I'm confused, because those tests were passing locally before, but now aren't. Maybe I forgot to commit some changed files, let's see... ah yeah, weird. Ok, let's give it a try now. |
dc80d9e
to
e82d5ad
Compare
e82d5ad
to
4cda4b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passed! Can you summarise if this implementation has any limitations and if there's any follow up work to be done?
Hooray!
I don't believe there's more work to be done. The main limitation:
I'm happy to update this PR with a docs update to that effect if you can point me to a good place in the docs to document this. |
These are generic constraints of cookies though, right? And you can always modify the underlying request to include credentials if you need to tinker with settings. So in that sense there's no extra need to document this? |
Yes, that's true. |
Closes #1621
Related to #231