Conversation
d11d1a0 to
83386cf
Compare
VegetarianOrc
left a comment
There was a problem hiding this comment.
Code looks okay to me! A few comments throughout.
Left a couple of comments to help me understand the motivation for the structure of the samples as well.
Would like to clear up the intention around removing the sync operation sample before approving.
There was a problem hiding this comment.
Was this same intentionally deleted? I don't think we should remove the basic sync operation example in favor of a messaging sample.
There was a problem hiding this comment.
So this was the original base that I used as a starting point as it had messaging - everything that was in it is now in the new sample. I just moved the package name to be consistent with what I was doing elsewhere.
You have a point about basic sync operations - but this already had messaging in it so it was already not a basic sync example? So I think this is OK, but I can also see it being debatable, so leaving this comment unresolved so you can reply.
There was a problem hiding this comment.
As long as we update the related doc about this to reference this new sample, I think it's probably fine given the context!
In fact, we probably should take a pass on those docs for each lang as these samples get merged in to ensure they reflect the new sample, links are valid, etc.
| The caller workflow: | ||
| 1. Queries for supported languages (`get_languages` -- backed by a `@workflow.query`) | ||
| 2. Changes the language to Arabic (`set_language` -- backed by a `@workflow.update` that calls an activity) | ||
| 3. Confirms the change via a second query (`get_language`) | ||
| 4. Approves the workflow (`approve` -- backed by a `@workflow.signal`) |
There was a problem hiding this comment.
Nit: The caller workflow only executes nexus operations and it feels a bit confusing to call out that they're backed by workflow.query/update/signal. IMO that info should be called out about or alongside the service handler.
There was a problem hiding this comment.
Since this is demonstrating queries, updates and signals this does actually feel useful to me though
This is sample code to show two ways to send messages (signals, queries, and updates) through Nexus.