Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add workflow quickstart for JS #989

Merged
merged 21 commits into from
Feb 27, 2024
Merged

Add workflow quickstart for JS #989

merged 21 commits into from
Feb 27, 2024

Conversation

kaibocai
Copy link
Contributor

Description

Please explain the changes you've made

Add workflow quickstart for JS

Issue reference

resolve #964

We strive to have all PRs 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:

  • The quickstart code compiles correctly
  • You've tested new builds of the quickstart if you changed quickstart code
  • You've updated the quickstart's README if necessary
  • If you have changed the steps for a quickstart be sure that you have updated the automated validation accordingly. All of our quickstarts have annotations that allow them to be executed automatically as code. For more information see mechanical-markdown. For user guide with examples see Examples.

MregXN and others added 17 commits November 21, 2023 11:22
@MregXN
use InvokeHttpClient in service invocation csharp sample
3d9799f 
Signed-off-by: MregXN <[email protected]>
@MregXN
add dependency
cb2075f 
Signed-off-by: MregXN <[email protected]>
@MregXN
retrigger to pass validation
5097d9a 
Signed-off-by: MregXN <[email protected]>
@MregXN
use daprWorkflowClient
0453824 
Signed-off-by: MregXN <[email protected]>
@WhitWaldo
Clarified the actor state operations performed and what subsequent ac…
48842ed 
…tivities must be performed by the user for persistance. Previous wording was confusing.

signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo
Updated comment to reflect call out on Discord by Pundil
cea9916 
Signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo
Updated comment to reflect call out on Discord by Pundil
0f93b1d 
Signed-off-by: Whit Waldo <[email protected]>
@WhitWaldo
Merge remote-tracking branch 'origin/updated-actor-csharp-quickstart-…
b6a1a5e 
…language' into updated-actor-csharp-quickstart-language
@paulyuk
Merge pull request dapr#979 from paulyuk/release-1.13
334b35a 
Fix tests broken by timing change
@paulyuk
Revert "Fix tests broken by timing change"
873eecb
@paulyuk
Merge pull request dapr#983 from dapr/revert-979-release-1.13
cfc5de1 
Revert "Fix tests broken by timing change"
@paulyuk
Merge pull request dapr#902 from MregXN/use-daprclient
583e9b3 
modify Service Invocation sample
@paulyuk
Merge pull request dapr#961 from WhitWaldo/updated-actor-csharp-quick…
d718efe 
…start-language

Clarified actor state persistence description
@paulyuk
Merge pull request dapr#959 from MregXN/remove-daprClient-for-workflow
a933afe 
use daprWorkflowClient instead of daprClient in workflow sample
@mikeee
bump go to 1.21
e0ec38b 
Signed-off-by: mikeee <[email protected]>
@mikeee
bump short timeouts
474c283 
Signed-off-by: mikeee <[email protected]>
@paulyuk
Merge pull request dapr#986 from mikeee/bump-go-validation
ec1e515 
bump go validation workflow to 1.21
@kaibocai
add quickstart for JS
2d7b401 
Signed-off-by: kaibocai <[email protected]>

clean up

Signed-off-by: kaibocai <[email protected]>

update .gitignore

fix .gitignore

Signed-off-by: kaibocai <[email protected]>
This was referenced Feb 16, 2024
Closed
Merged
@paulyuk paulyuk requested a review from cgillum February 18, 2024 06:22
@paulyuk paulyuk added this to the 1.13 milestone Feb 18, 2024
@paulyuk paulyuk added P1 language/javascript Pull requests that update Javascript code labels Feb 18, 2024
@kaibocai
fix readme check
5cfcd0e 
Signed-off-by: kaibocai <[email protected]>
@elena-kolevska
Copy link

@msfussell @cgillum Could we get a review on this pls? 🙏

cgillum
cgillum reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback.

workflows/javascript/sdk/order-processor/orderProcessingWorkflow.ts Outdated Show resolved Hide resolved
workflows/javascript/sdk/order-processor/orderProcessingWorkflow.ts

if (orderPayLoad.totalCost > 5000) {
const approvalResult = yield ctx.callActivity(requestApprovalActivity, orderPayLoad);
if (!approvalResult) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the request for approval is sent, we're supposed to wait for an external event to get the approval result. We can't realistically rely on an activity for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the csharp and java quickstart examples when creating it for JS, I see both csharp and java are not waiting for an external event here. I will work on adding external event logic here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cgillum @kaibocai like this suggestion, just can we move this to a new issue, and apply to all languages? given 1.13 is about to close I think we can do this async and not block on it. Sound ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me Thanks!

workflows/javascript/sdk/order-processor/workflowApp.ts Outdated Show resolved Hide resolved
workflows/javascript/sdk/order-processor/workflowApp.ts Outdated Show resolved Hide resolved
workflows/javascript/sdk/order-processor/workflowApp.ts Show resolved Hide resolved
workflows/javascript/sdk/order-processor/workflowApp.ts Outdated Show resolved Hide resolved
@kaibocai
minor refactory
9aecc35 
Signed-off-by: kaibocai <[email protected]>
@kaibocai kaibocai mentioned this pull request Feb 27, 2024
Closed
paulyuk
paulyuk approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@paulyuk paulyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Suggest we move the idea on external calls to another issue, that is optional for 1.13, and we merge this now as-is.

@paulyuk paulyuk changed the base branch from master to release-1.13 February 27, 2024 18:11
@paulyuk paulyuk merged commit 7429289 into dapr:release-1.13 Feb 27, 2024
6 of 7 checks passed
This was referenced Feb 27, 2024
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgillum cgillum cgillum left review comments

@paulyuk paulyuk paulyuk approved these changes

@msfussell msfussell Awaiting requested review from msfussell

Assignees

@kaibocai kaibocai

Labels
area/workflow language/javascript Pull requests that update Javascript code P1
Projects
None yet
Milestone
1.13
Development

Successfully merging this pull request may close these issues.

[Workflow] Quickstart for JavaScript SDK
7 participants
@kaibocai @elena-kolevska @paulyuk @cgillum @MregXN @WhitWaldo @mikeee