-
Notifications
You must be signed in to change notification settings - Fork 54
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
Runnable new-and-enhanced JS samples directory for v3 #438
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some preliminary comments. Thank you for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the overall structure of this PR. I skimmed the code, but don't feel the samples directory is worth a detailed review from me personally. As mentioned offline, my favorite thing about this PR is that it got you to test a lot of scenarios 🙂 Good work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although it seems you have some merge conflicts to resolve. Still, the contents of this PR seem good to me.
This PR tackles a few issues, mainly resolves #421, but it also addresses #357, #356, and partially #276. Also provides good scaffolding for #394. This PR migrates the old samples and adds a new, runnable, new programming model v3-compatible
samples-js
directory. This PR is only for the JS samples, but I will create another one for another, separate TS directory. I also will delete the existing samples directory in a separate PR. In addition, this PR:moment
(replaced withluxon
),request
(replaced withaxios
), and the deprecated wunderground weather API (replaced with OpenWeatherMap API), and maybe othersI tested all the samples manually and verified that they all work, with some notable exceptions:
smsPhoneVerification
sample -- this revealed an issue on the framework level with activities having extra output bindings -- issue here: Better handle return value when not set by a binding azure-functions-nodejs-library#53 to tracksayHelloWithSubOrchestrator
sample -- there is an issue with added double quotes -- issue here Double quotes added to input passed from suborchestrator to activity #436 to trackshoppingEntity
/shoppingOrchestration
samples -- entities don't support non-JSON serializable states -- issue here Entities do not support non-JSON serializable states #437 to tracklistAzureSubscriptions
sample -- I couldn't test this locallyOfc, any design/organization/naming choices made here are subject to discussion :)