fix(medusa): API workflow subscription - #15134
Conversation
🦋 Changeset detectedLatest commit: 2b92733 The changes in this PR will be included in the next version bump. This PR includes changesets to release 78 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 |
|
@v0eak is attempting to deploy a commit to the medusajs Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for your contribution, @v0eak! After reviewing this PR, we need a few things addressed before we can move forward: Required changes:
Potential Bugs:
if (event.event_type === "onFinish") {
onWorkflowFinishSpy() // should be onWorkflow2FinishSpy()
resolve()
}This is a copy-paste error from the Notes (non-blocking):
Triggered by: new PR opened |
|
Thank you for updating the PR description, @v0eak! The previously required change is still outstanding: Required changes:
Potential Bugs:
if (event.event_type === "onFinish") {
onWorkflowFinishSpy() // should be onWorkflow2FinishSpy()
resolve()
}
Notes (non-blocking):
Triggered by: PR description updated |
|
Thank you for your contribution, @v0eak! 🎉 After an initial review, this PR looks good to us. Here's a summary: ✅ PR template is complete The previously required fix (wrong spy in A team member will do a final review before this is merged. We appreciate your patience! Notes (non-blocking):
Triggered by: new commit pushed |
|
Thank you for the continued updates, @v0eak! After reviewing the latest commit, a new issue has been introduced that needs to be addressed before we can restore initial approval. Required changes:
Potential Bugs:
// In the new success test (same values as the failure test above it):
const workflowId = 'test-workflow'
const transactionId = 'test-transaction'The failure test also uses const workflowId = 'test-workflow-success-' + Math.random().toString(36).substring(2, 15)
const transactionId = 'test-transaction-' + Math.random().toString(36).substring(2, 15)Notes (non-blocking):
Triggered by: new commit pushed |
|
I have removed the http test because internally the test runner uses axios, and its not setup to allow for SSE. It would require changes to the medusa-test-runner (maybe reason why the test was missing in the first place?) |
|
Thank you for addressing all the required changes, @v0eak! 🎉 After reviewing the latest commit, this PR looks good to us. Here's a summary: ✅ PR template is complete A team member will do a final review before this is merged. We appreciate your patience! Notes (non-blocking):
Triggered by: new commit pushed |
shahednasser
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Aside from the comments below, please remove all documenation changes from this PR. Instead, open a separate PR with the documentation change in www/apps/resources/app/js-sdk/page.mdx. This ensures the documentation change is only merged when the change in this PR is released. Other changes related to the OpenAPI specs are not necessary as thy're automatically generated on the next release.
|
Thank you for the continued updates, @v0eak! The latest commit removes the auto-generated spec files ( ✅ Notes (non-blocking):
Triggered by: new commit pushed |
|
Thank you for your continued updates, @v0eak! After reviewing the latest commit, this PR remains approved. All previously required changes have been resolved. ✅ One correction to a note carried across previous reviews: the A team member will do a final review before this is merged. We appreciate your patience! Notes (non-blocking):
Triggered by: new commit pushed |
Summary
What —
to /api/admin/workflows-executions/[workflow_id]/[transaction_id]/subscribe/route.ts, because it is not possible to subscribe to individual steps, only to individual workflows.
Why —
Current implementation relies on req.query to get the id of workflow/transaction, but it should get them from req.params instead. It is also impossible to subscribe to specific steps.
When setting up SSE streams with JS-SDK, if an event has yet to be streamed, it is impossible to abort, because nothing has returned from the JS-SDK yet.
How —
Change req.query to req.params
fetchStream now always returns (non-null generator), so that it is possible to abort before receiving any events
Testing —
https://docs.medusajs.com/api/admin#workflows-executions_getworkflowsexecutionsworkflow_idsubscribe
Examples
https://docs.medusajs.com/resources/js-sdk#stream-server-sent-events
Checklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
This is a new PR based on 13886.
Unfortunately I synced the fork and deleted my commits, which then automatically closed the PR.
Closes #15135
Closes #15136