Backwards incompatible changes #4
Conversation
now, it will be possible to extend the serve method in the future as it returns const { POST } = serve(...).
it isn't changed in hono, as it doesn't fit the hono syntax.
examples/hono/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@upstash/qstash": "latest", | ||
| "@upstash/workflow": "^0.1.0", |
There was a problem hiding this comment.
Shouldnt we get the package from ../../dist as above?
There was a problem hiding this comment.
updated all imports with canary
| { | ||
| destination: WORKFLOW_ENDPOINT, | ||
| headers: { | ||
| "upstash-feature-set": "WF_NoDelete", |
There was a problem hiding this comment.
What does this header do? Just to know
There was a problem hiding this comment.
In @upstash/qstash, if the request in context.call failed, the workflow run would fail and failure handlers would be called.
Now, we don't want to make the workflow run fail, but we want to continue from where we left off. This header tells the qstash-server that this workflow is from the new sdk, and run shouldn't fail
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters | ||
| public async call<TResult = unknown, TBody = unknown>( | ||
| public async call( |
There was a problem hiding this comment.
Why did we remove the generic typing here? I think it's nice to have the option of const res = call<T>() instead of const res = call() as T
There was a problem hiding this comment.
to get rid of no-unnecessary-type-parameters warning
There was a problem hiding this comment.
We could also disable that rule for this page
There was a problem hiding this comment.
Yusuf was suggesting removing it, how about removing it for now and adding it later?
We can add it later but can't remove it later. I want to think about this a bit more because there is no generic in await request.json() for instance
src/context/context.ts
Outdated
| body?: TBody, | ||
| headers?: Record<string, string> | ||
| ) { | ||
| callSettings: { |
There was a problem hiding this comment.
I think we can call this just settings
src/context/context.ts
Outdated
| try { | ||
| const body = JSON.parse(result); | ||
| return { | ||
| status: -1, |
There was a problem hiding this comment.
Why do we return status -1 on succesful request?
There was a problem hiding this comment.
it could be anything from 200 to 299, so I didn't want to return anything, but let's go with 200.
| * @returns A promise that resolves to a response. | ||
| */ | ||
| const handler = async (request: TRequest) => { | ||
| await debug?.log("INFO", "ENDPOINT_START"); |
There was a problem hiding this comment.
it indicates that the endpoint was called and is executing
| notifyResponse: NotifyResponse[]; | ||
| }; | ||
|
|
||
| export type CallResponse = { |
There was a problem hiding this comment.
I think this should have generics
There was a problem hiding this comment.
you mean for body field right? This is the same issue as whether we want to have await context.call<SomeType>. I would like to think a bit more before adding this
as we migrate from @upstash/qstash, there are some backwards incompatible changes we want to make.
These will be explained in the migration guide. To briefly explain them here:
const { POST } = serve(...)