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.

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

Add workflow quickstart for JS #989

Merged
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
Signed-off-by: MregXN <mregxn@gmail.com>
Signed-off-by: MregXN <mregxn@gmail.com>
Signed-off-by: MregXN <mregxn@gmail.com>
Signed-off-by: MregXN <mregxn@gmail.com>
…tivities must be performed by the user for persistance. Previous wording was confusing.

signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whitson.waldo@gmail.com>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…language' into updated-actor-csharp-quickstart-language
Fix tests broken by timing change
Revert "Fix tests broken by timing change"
modify Service Invocation sample
…start-language

Clarified actor state persistence description
use daprWorkflowClient instead of daprClient in workflow sample
Signed-off-by: mikeee <hey@mike.ee>
Signed-off-by: mikeee <hey@mike.ee>
bump go validation workflow to 1.21
Signed-off-by: kaibocai <kaibocai@microsoft.com>

clean up

Signed-off-by: kaibocai <kaibocai@microsoft.com>

update .gitignore

fix .gitignore

Signed-off-by: kaibocai <kaibocai@microsoft.com>
@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
Signed-off-by: kaibocai <kaibocai@microsoft.com>
@elena-kolevska
Copy link

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

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.


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 Outdated Show resolved Hide resolved
Signed-off-by: kaibocai <kaibocai@microsoft.com>
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
Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@paulyuk paulyuk merged commit 7429289 into dapr:release-1.13 Feb 27, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/workflow language/javascript Pull requests that update Javascript code P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Workflow] Quickstart for JavaScript SDK
7 participants