-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Deprecate simple objects from endpoints #8132
Conversation
🦋 Changeset detectedLatest commit: 81a7056 The changes in this PR will be included in the next version bump. 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 |
export type EndpointOutput = | ||
| { | ||
body: Body; | ||
encoding?: Exclude<BufferEncoding, 'binary'>; | ||
encoding?: BufferEncoding; | ||
} | ||
| { | ||
body: Uint8Array; |
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.
I think this was a mistake in #8003, we technically only support binary strings, and not binary unit8arrays. We have a test for binary strings and the PR caused a type error.
But simple objects are being deprecated anyways so I don't think it's a big deal.
type: 'simple', | ||
cookies: context.cookies, | ||
}; | ||
let body: BodyInit; |
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.
All the pipeline simple object handling converges into this function starting from this line. This should also make the dev/prod behaviour more consistent.
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.
I love how much churn we removed, and we consolidated one way to process the responses from the endpoints!
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.
Looks great, @bluwy !
As for the rest of docs, when you file an issue for this, please add a note there to remind us (inclusive! anyone can jump in here!) to do a "find all" instances of "Response" in the docs, then examine the surrounding context and update any text descriptions or code samples as needed! 🙌
Changes
callEndpoint
. This makes the#endpointHandler
convention in Pipeline not used much (onlyssrPipeline
has a special case) cc @ematipicoTesting
Added test for
ResponseWithEncoding
, and tests for previous object form so they still work, until they're removed.Docs
Added changeset. Any mention of the object form in docs might need to be removed.
/cc @withastro/maintainers-docs for feedback!