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

(AzureCXP) fixed continueAsNew for JavaScript #71189

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

ChaitanyaNaykodi-MSFT
Copy link
Contributor

resolves MicrosoftDocs/azure-docs#70981

resolves MicrosoftDocs/azure-docs#70981
@PRMerger19
Copy link
Contributor

@ChaitanyaNaykodi-MSFT : Thanks for your contribution! The author(s) have been notified to review your proposed change.

@cgillum
Copy link
Contributor

cgillum commented Feb 25, 2021

@davidmrdavid is this fix correct? I thought we didn't need to yield continueAsNew(), but maybe the JavaScript SDK is doing something I didn't expect?

@ktoliver ktoliver added aq-pr-triaged tracking label for the PR review team TriagedFeb2021 labels Feb 25, 2021
@ktoliver
Copy link
Contributor

Note that publishing in this repository is suspended until Tuesday, March 2. Merging will resume Tuesday. Authors can still review and sign off on PRs during this time. Thanks.

@davidmrdavid
Copy link
Contributor

davidmrdavid commented Feb 25, 2021

This is right, @cgillum.
In Python, where I contributed to continue_as_new, we handle both the case where the API is yield'ed and the case where it is simply called. In both cases, it works as expected.

We needed to handle both cases because JS already had the precedent of requiring a yield for continueAsNew, which departs from the C# style. In other words, it had the requirement implied by this PR.

I believe that behavior in JS was unintended, and that we wanted to fix it to support both cases, like Python, but that we never followed-up on that. That's the tricky thing with OOProc, bugs need to be followed-up across repos....

That aside, I believe we should merge this fix and simply create a ticket to ensure we finally fix continueAsNew in JS to be like in Python. Once we complete that item, we can tweak the docs once more.

@davidmrdavid
Copy link
Contributor

I created the JS item here: Azure/azure-functions-durable-js#248

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.

Thanks @davidmrdavid for the confirmation. In that case, I think we're good to merge this PR.

@ChaitanyaNaykodi-MSFT
Copy link
Contributor Author

Thank you @cgillum & @davidmrdavid

@ChaitanyaNaykodi-MSFT
Copy link
Contributor Author

#sign-off

@PRMerger15
Copy link
Contributor

@ChaitanyaNaykodi-MSFT: I'm sorry - only the author of this article, @cgillum, can sign off on your changes. But we do have an exception process - if you are on the Microsoft content or product team for this product area, you can ask the PR review team to review and merge it by sending mail to the techdocprs alias.

@davidmrdavid
Copy link
Contributor

#sign-off

@PRMerger20
Copy link
Contributor

@davidmrdavid: I'm sorry - only the author of this article, @cgillum, can sign off on your changes. But we do have an exception process - if you are on the Microsoft content or product team for this product area, you can ask the PR review team to review and merge it by sending mail to the techdocprs alias.

@davidmrdavid
Copy link
Contributor

@cgillum, do you mind doing the sign-off?

@cgillum
Copy link
Contributor

cgillum commented Feb 26, 2021

#sign-off

@ktoliver
Copy link
Contributor

@cgillum Thanks for the sign-off. We'll merge this approved pull request on Tuesday when publishing in this repo resumes.

@ktoliver ktoliver merged commit a1359ee into master Mar 4, 2021
@tfosmark tfosmark deleted the ChaitanyaNaykodi-MSFT-patch-1 branch March 5, 2022 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typo on Page section: Periodic work example
7 participants