refactor: mv durabletask-js into sdk#738
Conversation
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #738 +/- ##
============================================
- Coverage 100.00% 41.54% -58.46%
============================================
Files 1 46 +45
Lines 6 5736 +5730
Branches 1 225 +224
============================================
+ Hits 6 2383 +2377
- Misses 0 3309 +3309
- Partials 0 44 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the durabletask-js implementation into the JS SDK (src/workflow/internal/durabletask), updates SDK/workflow code and tests to use the internal implementation, and removes the external @dapr/durabletask-js dependency.
Changes:
- Vendored Durable Task client/worker/task runtime code into
src/workflow/internal/durabletaskand rewired workflow runtime/client imports to use it. - Added unit tests for the vendored orchestration/activity executors and adjusted test scripts to include
*.spec.ts. - Updated build packaging to copy vendored protobuf JS/typings into
build/and removed@dapr/durabletask-jsfrom dependencies.
Reviewed changes
Copilot reviewed 55 out of 59 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/workflow/workflowRuntimeStatus.test.ts | Updates enum import to internal durabletask path. |
| test/unit/workflow/durabletask/orchestration_executor.spec.ts | Adds orchestration executor unit tests for the vendored runtime. |
| test/unit/workflow/durabletask/activity_executor.spec.ts | Adds activity executor unit tests for the vendored runtime. |
| test/e2e/workflow/workflow.test.ts | Updates Task import to internal durabletask path. |
| src/workflow/runtime/WorkflowRuntimeStatus.ts | Switches OrchestrationStatus import to internal durabletask. |
| src/workflow/runtime/WorkflowRuntime.ts | Switches durabletask worker/context imports to internal durabletask. |
| src/workflow/runtime/WorkflowContext.ts | Switches Task/whenAll/whenAny imports to internal durabletask. |
| src/workflow/runtime/WorkflowActivityContext.ts | Switches ActivityContext import to internal durabletask. |
| src/workflow/internal/durabletask/worker/task-hub-grpc-worker.ts | Adds internal worker to poll sidecar work-items and execute orchestrations/activities. |
| src/workflow/internal/durabletask/worker/runtime-orchestration-context.ts | Adds runtime orchestration context implementation (timers, activities, events, continue-as-new). |
| src/workflow/internal/durabletask/worker/registry.ts | Adds internal registry for orchestrators/activities. |
| src/workflow/internal/durabletask/worker/orchestration-executor.ts | Adds orchestration executor that replays history and emits actions. |
| src/workflow/internal/durabletask/worker/orchestration-execute-result.ts | Adds small result wrapper for executor outputs. |
| src/workflow/internal/durabletask/worker/index.ts | Adds worker helper utilities (non-determinism errors, summaries, suspendable detection). |
| src/workflow/internal/durabletask/worker/exception/stop-iteration-error.ts | Adds StopIterationError used for generator completion. |
| src/workflow/internal/durabletask/worker/exception/orchestrator-not-registered-error.ts | Adds OrchestratorNotRegisteredError. |
| src/workflow/internal/durabletask/worker/exception/activity-not-registered-error.ts | Adds ActivityNotRegisteredError. |
| src/workflow/internal/durabletask/worker/activity-executor.ts | Adds activity execution logic for sidecar work-items. |
| src/workflow/internal/durabletask/utils/promise.util.ts | Adds a simple promise detection helper. |
| src/workflow/internal/durabletask/utils/pb-helper.util.ts | Adds helpers for creating history events/actions and failure details. |
| src/workflow/internal/durabletask/utils/enum.util.ts | Adds enum value→key helper. |
| src/workflow/internal/durabletask/types/output.type.ts | Adds TOutput type alias. |
| src/workflow/internal/durabletask/types/orchestrator.type.ts | Adds orchestrator function type alias. |
| src/workflow/internal/durabletask/types/input.type.ts | Adds TInput type alias. |
| src/workflow/internal/durabletask/types/activity.type.ts | Adds activity function type alias. |
| src/workflow/internal/durabletask/task/when-any-task.ts | Adds WhenAnyTask composite task. |
| src/workflow/internal/durabletask/task/when-all-task.ts | Adds WhenAllTask composite task. |
| src/workflow/internal/durabletask/task/task.ts | Adds Task base class. |
| src/workflow/internal/durabletask/task/index.ts | Adds whenAll/whenAny/getName helpers. |
| src/workflow/internal/durabletask/task/failure-details.ts | Adds FailureDetails model. |
| src/workflow/internal/durabletask/task/exception/task-failed-error.ts | Adds TaskFailedError wrapping protobuf failure details. |
| src/workflow/internal/durabletask/task/exception/orchestration-state-error.ts | Adds OrchestrationStateError. |
| src/workflow/internal/durabletask/task/exception/non-determinism-error.ts | Adds NonDeterminismError. |
| src/workflow/internal/durabletask/task/context/orchestration-context.ts | Adds OrchestrationContext abstract API surface. |
| src/workflow/internal/durabletask/task/context/activity-context.ts | Adds ActivityContext. |
| src/workflow/internal/durabletask/task/composite-task.ts | Adds CompositeTask parent for composite tasks. |
| src/workflow/internal/durabletask/task/completable-task.ts | Adds CompletableTask for completing/failing tasks. |
| src/workflow/internal/durabletask/proto/orchestrator_service_grpc_pb.js | Adds vendored generated gRPC service JS. |
| src/workflow/internal/durabletask/proto/orchestrator_service_grpc_pb.d.ts | Adds vendored generated gRPC service typings. |
| src/workflow/internal/durabletask/orchestration/orchestration-state.ts | Adds OrchestrationState model and failure raising helper. |
| src/workflow/internal/durabletask/orchestration/orchestration-purge-result.ts | Adds PurgeResult model. |
| src/workflow/internal/durabletask/orchestration/orchestration-purge-criteria.ts | Adds PurgeInstanceCriteria model. |
| src/workflow/internal/durabletask/orchestration/index.ts | Adds helper to build OrchestrationState from protobuf response. |
| src/workflow/internal/durabletask/orchestration/exception/orchestration-failed-error.ts | Adds OrchestrationFailedError. |
| src/workflow/internal/durabletask/orchestration/enum/orchestration-status.enum.ts | Adds orchestration status enum + protobuf converters. |
| src/workflow/internal/durabletask/index.ts | Exposes internal durabletask client/worker/context entrypoints. |
| src/workflow/internal/durabletask/exception/timeout-error.ts | Adds TimeoutError used by client waits. |
| src/workflow/internal/durabletask/client/client.ts | Adds TaskHubGrpcClient for scheduling/waiting/raising/purging workflows. |
| src/workflow/internal/durabletask/client/client-grpc.ts | Adds basic gRPC stub creation helper for durabletask sidecar. |
| src/workflow/client/WorkflowState.ts | Switches to internal durabletask OrchestrationState import. |
| src/workflow/client/WorkflowFailureDetails.ts | Switches to internal durabletask FailureDetails import. |
| src/workflow/client/DaprWorkflowClient.ts | Switches to internal durabletask TaskHubGrpcClient import. |
| src/types/workflow/Workflow.type.ts | Switches Task type import to internal durabletask. |
| src/index.ts | Switches exported Task type import to internal durabletask. |
| scripts/build.sh | Copies vendored durabletask protobuf JS/typings into build output. |
| package.json | Removes @dapr/durabletask-js dependency; updates workflow unit test glob. |
| package-lock.json | Removes @dapr/durabletask-js dependency entries; updates package version metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
|
Hello @sicoyle! Once you're able to get the e2e tests passing, I'd be happy to merge this. It's my goal for 1.18 to migrate this to use TestContainers at which point I'd be happy to revisit and supply additional e2e testing. Further, do you know if, as part of this migration, the implementation strictly uses the code used by the Dapr protos implementation or whether this is just effectively a rewrite to avoid the dependency (but is potentially still riddled with unnecessary paths - e.g. to durable objects)? |
|
Also I saw 3 duplicate
Maybe we can consolidate this into a shared util? |
🤠 👋 @WhitWaldo Heck yeah 🙌 Coooool, I'm glad we're aligned on simplifying the SDKs and moving durabletask into their corresponding SDK repos. I believe you've essentially done the same for dotnet too here in past right? This migration strictly uses the Dapr/gRPC code paths only. It is not me rewriting the JS implementation of Durabletask by any means. This is just the minimum subset of durabletask JS code vendored in and combined in-tree here to rm the external dependency and extra hop for SDK maintainers. |
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
| stream.on("data", (workItem: pb.WorkItem) => { | ||
| if (this._activeWorkItems >= this._maxConcurrentWorkItems) { | ||
| this.logger.warn( | ||
| `Max concurrent work items (${this._maxConcurrentWorkItems}) reached, skipping work item`, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (workItem.hasOrchestratorrequest()) { | ||
| this.logger.info( | ||
| `Received "Orchestrator Request" work item with instance id '${workItem | ||
| ?.getOrchestratorrequest() | ||
| ?.getInstanceid()}'`, | ||
| ); | ||
| this._trackWorkItem( | ||
| this._executeOrchestrator(workItem.getOrchestratorrequest() as any, client.stub), | ||
| ); | ||
| } else if (workItem.hasActivityrequest()) { | ||
| this.logger.info(`Received "Activity Request" work item`); | ||
| this._trackWorkItem( | ||
| this._executeActivity(workItem.getActivityrequest() as any, client.stub), | ||
| ); | ||
| } else { | ||
| this.logger.warn(`Received unknown work item`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
_trackWorkItem is async. Shouldn't we await those calls or have a .catch()? If it throws, the unhandled rejection can crash.
We need to do something like this:
this._trackWorkItem(
this._executeOrchestrator(workItem.getOrchestratorrequest() as any, client.stub),
).catch((err) => {
this.logger.error(`Unhandled error in work item execution: ${err}`);
});
|
Another suggestion regarding Downgrade replay and per-event messages to // orchestration-executor.ts:62
this.logger.debug(`${instanceId}: Rebuilding local state with ${oldEvents.length} history events...`);
// orchestration-executor.ts:71
this.logger.debug(`${instanceId}: Processing ${newEvents.length} new history event(s)`);
// task-hub-grpc-worker.ts:167
this.logger.debug(`Received orchestrator request for instance '${instanceId}'`); |
|
Another one where Claude found these inconsistencies, where you are using loose equality:
We should replace all with |
|
@ConstantinChirila Please only change loose equality on methods for which there is a test that proves it has no net effect from how it was originally implemented (or has an E2E test showing it doesn't break anything). Given this is largely a copy from the DurableTask implementation, I don't want to fix something for syntactical correctness just to find out that a blind fix breaks something. |
What I wrote in .NET was a "clean room" rewrite of Workflow to reflect what the protos called for based on how I understood workflows to work (based on reading the runtime) and was not a mere migration of DurableTask into the SDK. It was an enormous amount of effort though, but led to some dramatic improvements in the SDK shape and feel. I haven't dug through the JS DurableTask implementation in a bit, but evaluating rewriting the client to remove DurableTask completely can certainly be the focus of another PR. I'll try to work through this PR this week and get back to you with a review of any other changes! |
Signed-off-by: Samantha Coyle <sam@diagrid.io>
I added tests for all of the changes :) |
Signed-off-by: Samantha Coyle <sam@diagrid.io>
|
Looks like it's not building. I know the E2E tests are flaky, so I can re-run those a few times and try to see if it'll clear, but if you can fix the build issue, I'd appreciate it. |
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
|
@WhitWaldo I went ahead and fixed some of the great comments from constantin as there were so fundamental issues that got pointed out. I did not go crazy there and change a ton, I added tests forward on those that I did address. Build should be g2g now 🙌 and pls disregard the codecov failures as those are existing gaps in durabletask-js that we can address later on. |
I'm not stressing about any of the E2E tests at this point - if you can resolve the merge conflicts, I'll get this merged in. |
Signed-off-by: Samantha Coyle <sam@diagrid.io>
…refactor/mv-dt-here
Signed-off-by: Samantha Coyle <sam@diagrid.io>
|
this should be g2g @WhitWaldo thanks for your patience 😁 |
Doesn't look like it's happy with your header comments.. https://github.com/dapr/js-sdk/actions/runs/24421385731/job/71344254689?pr=738 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 69 changed files in this pull request and generated 9 comments.
Files not reviewed (2)
- examples/workflow/authoring/package-lock.json: Language not supported
- examples/workflow/management/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Advanced Options */ | ||
| "skipLibCheck": false /* Skip type checking of declaration files. */, | ||
| "skipLibCheck": true /* Skip type checking of declaration files. */, | ||
| "forceConsistentCasingInFileNames": true /* Disallow inconsistently-cased references to the same file. */ |
There was a problem hiding this comment.
Changing skipLibCheck to true weakens this TypeScript build e2e test because it can hide broken .d.ts files in the published package (including the newly vendored durabletask protos). If this was added to work around type issues, it would be better to fix the underlying declaration errors and keep skipLibCheck=false for this validation project.
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
Signed-off-by: Samantha Coyle <sam@diagrid.io>
It wants dapr header first, should be resolved now :) |
|
Merging despite failing e2e tests as I'm going to get those resolved in a separate PR after the fact before next release |
WhitWaldo
left a comment
WhitWaldo
left a comment
There was a problem hiding this comment.
Looks great - thank you for taking the time to do this work!
Description
I went through the process of updating to migrate durabletask-python into the python-sdk here dapr/python-sdk#963
I started it manually and then used claude to help fix things and refactor a bit nicer. I then had claude create a migration plan using the learnings we had from the python migration to give more context to iterate this out with subagents to the other SDKs. This is the corresponding JS SDK update :)
We should then archive the durabletask-js repo pls.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: