feat: Allow action callbacks to return a result value#217
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces explicit action result typing: schemas split into with-result / no-result variants, action-definition callbacks refactored to reflect return types, host execution/result shapes made discriminated unions, and host action metadata now includes Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/host/src/internal/actions.ts (1)
63-63: Nice touch on capturing the result! The initialization at line 63 is already type-safe sinceCompanionVariableValuealready includesundefined. Adding an explicit initializer like= undefinedis a nice-to-have for clarity—it makes the "not set" default more obvious to readers, especially if your TypeScript configuration is strict about definite assignment.Optional improvement
- let result: CompanionVariableValue + let result: CompanionVariableValue | undefined = undefinedEither way, the code works great as-is. Thanks for this addition!
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73207dac-f006-407e-918c-20282a31ddae
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/base/src/module-api/action.tspackages/base/src/module-api/base.tspackages/base/src/module-api/preset/definition.tspackages/host/package.jsonpackages/host/src/instance.tspackages/host/src/internal/actions.ts
jswalden
force-pushed
the
action-callback-return-result
branch
from
e3070d9 to
3f0f0bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0616082-14bc-441d-adf3-6d0bf56e2008
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/base/src/module-api/action.tspackages/base/src/module-api/base.tspackages/base/src/module-api/preset/definition.tspackages/host/package.jsonpackages/host/src/instance.tspackages/host/src/internal/actions.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/host/package.json
- packages/host/src/internal/actions.ts
- packages/base/src/module-api/base.ts
- packages/base/src/module-api/preset/definition.ts
jswalden
force-pushed
the
action-callback-return-result
branch
from
3f0f0bb to
f081104
Compare
|
This will fix #216. |
jswalden
changed the title
$(this:result). (#216)$(this:result)
jswalden
force-pushed
the
action-callback-return-result
branch
from
f081104 to
49b9666
Compare
|
I think the DX will be nicer if we take the return value of the callback instead of a method on the context. That will also make it clearer that this value must be provided before the method returns, and can't be provided after. As I said in the companion pr, I think that the action definition should also have a boolean property so that the action declares whether it does/might produce a value. so that we can provide some visual hints (either now or in the future) I'm also wondering about the types, but I think that I just need to have a play |
Oh, absolutely. But it's technically a breaking change, callbacks that returned something before abruptly will be exposing it. That's honestly the only reason I didn't make it work that way here.
If an exemplar value were added, then we could type-check that for I guess it's nice that this half of the thing, is somewhat independent of the user-exposed portion of this that can be finagled however, mostly separately. |
|
Oh, I guess if there's a boolean on offer, you could use that to decide whether to cast the return value to the result type or drop it, which would be treating it tantamount to the |
true, I suppose that an example value could be nice. But I'm hesistant to do any casting of the return value, as we often won't be able to (having done this for action/feedback options now as part of the builtin parsing). But maybe a boolean is a good thing to have to enable producing any return value, and an optional example or some definition of the return type (could just be 'number') could be a nice extra for those who want it. But yeah, otherwise that example/definition could be required, and used to enable the functionality. Then it can take the return value |
jswalden
force-pushed
the
action-callback-return-result
branch
from
49b9666 to
6ece8c1
Compare
|
The value of an example value would be -- if the other half of this were still exposing action results by |
|
I've done a bit of fiddling with the types. I'm thinking that accepting a small breaking change in In behringer-x32, it was one line to make it happy, homeassistant-server needs no changes. I was wondering about condensing the schema types, but after playing it does seem that split like this is better. |
|
FWIW there are some type-manipulation problems here (one that you introduced, but I think also one that I inadvertently introduced before anything you did!), and I'm working on fixing them. More when I've finished that. |
| ? Result extends JsonValue | ||
| ? CompanionActionDefinitionCallbackWithResult<Options, Result> | ||
| : CompanionActionDefinitionCallbackNoResult<Options> | ||
| : never |
There was a problem hiding this comment.
I find myself periodically infuriated with TypeScript's fairly careless disregard for how generics deal with types, versus unions of types, and how inserting an X extends any is the inscrutable syntax that breaks X into its constituent types while [X] extends [T] is how you avoid distributing while still doing the testing. Really seems like there should be much clearer, explicitly-distributive syntax. (Or nondistributive, I don't necessarily care about specifics, just they should use distinct syntax!)
That grump is relevant because something in the above causes Result to be correspondingly broken up (I think everything nested under a X extends T will have X as a union member) -- resulting in callback not being (...) => JsonValue | Promise<JsonValue> but instead (...) => string | number | null | ... | Promise<string> | Promise<number> | .... And then a callback that returns not one of the types within the JsonValue union, but a union of multiple of them, will fail to compile, as for action definitions in this test module:
return_value: {
name: 'Return a value',
options: [
{
id: 'val-to-return',
type: 'textinput',
label: 'Enter a JSON string to return its parsed value',
default: 'null',
useVariables: true,
},
],
hasResult: true,
callback: ({ options }): JsonValue => {
return JSON.parse(options['val-to-return'])
},
},Type '({ options }: CompanionActionEvent<{ 'val-to-return': string; }>) => JsonValue' is not assignable to type '((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => Promise | null) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => string | Promise) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => number | Promise) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => false | Promise) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => true | Promise) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => JsonObject | Promise) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => JsonValue[] | Promise<JsonValue[]>) | ((action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => readonly JsonValue[] | Promise<readonly JsonValue[]>)'.
Type '({ options }: CompanionActionEvent<{ 'val-to-return': string; }>) => JsonValue' is not assignable to type '(action: CompanionActionEvent<{ 'val-to-return': string; }>, context: CompanionActionContext) => Promise | null'.
Type 'JsonValue' is not assignable to type 'Promise | null'.
Type 'string' is not assignable to type 'Promise'.ts(2322)
action.d.ts(82, 5): The expected type comes from property 'callback' which is declared here on type 'CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, null> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, string> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, number> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, false> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, true> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, JsonObject> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, JsonValue[]> | CompanionActionDefinitionCallbackWithResult<{ 'val-to-return': string; }, readonly JsonValue[]>'
I created this goof for myself while working on the patch at one point, so it's easy to recognize. 🙂
This also reveals a failure of my own here: JsonValue | void is already a union, and there is no way to preserve JsonValue (or a union result extending it) without consistently using [TResult] extends [JsonValue] and similar to do testing on it. Which means that there's no letting JsonValue | void create a type that distributes into CAD<Options, JsonValue> | CAD<Options, void>. So every place that says CAD<Options, JsonValue | void> has to be changed to CAD<Options, JsonValue> | CAD<Options, void>. Or, as I prefer to overloading, CADWithResult<Options, JsonValue> | CADNoResult<Options> and similar. Passing through the most-generic layer where the result parameter is optional just feels more complicated to me than using the generics specific to result/no-result varieties.
The update I pushed will fix all this, and works with my test module. However, you may want to more deftly handle n/no-missing-import as happens with the type-testing helper module I added to perform tests -- since I presume you would be squashing/landing all this, if you have preferences for how best to handle that I'll let you implement them.
There was a problem hiding this comment.
maybe the type-testing stuff should be pulled out into a separate file, to make it clearer that it is test code?
not sure it is worth it
The reset of the comment about type unions here sounds familiar from battles Ive had in the past, and all I remember on this today is they are never fun to fix
There was a problem hiding this comment.
This is, of course, a matter of taste. But for testing entirely in the type system without impact on runtime code, I think it's generally desirable to keep the tests as close to the actual code being tested as possible. It's more readable to see the tests specifically of types, be specifically tested in the type system adjacent. After all, tsc boils it away during compilation, unlike testing code that actually does stuff at runtime.
| readonly #actionDefinitions = new Map<string, CompanionActionDefinition>() | ||
| readonly #actionDefinitions = new Map< | ||
| string, | ||
| CompanionActionDefinition<CompanionActionSchema<CompanionOptionValues, any>> |
There was a problem hiding this comment.
I think if you use JsonValue | void instead of any, at least with my CAD definition techniques this expands to CompanionActionDefinition<CompanionActionSchema<CompanionOptionValues, JsonValue> | CompanionActionSchema<CompanionOptionValues, void>> i.e. CAD<CAS<COV, JsonValue>> | CAD<CAS<COV, void>>. I think there is a possibility you're using any here because that doesn't result in a union being prematurely exploded on you.
This is gnarly enough that the expansion probably ought be actually verified. My preferred way would be to use type-testing to write out the type expectations and guarantee their enforcement at compile time, as I've done in the modules I work in -- although the first-pass hacking-in I did in the latest commit pushed here plays fast and loose importing 'type-testing' in ways you probably want to clean up. (I had to go out of my way to turn off n/no-missing-import for this Node module, and the eslint rule thing probably needs dealing with too.)
jswalden
changed the title
$(this:result)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e265d5dc-7fc9-494f-9f28-57c6d65dc2d2
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/base/package.jsonpackages/base/src/module-api/action.tspackages/base/src/module-api/base.tspackages/base/src/module-api/preset/definition.tspackages/host/src/context.tspackages/host/src/instance.tspackages/host/src/internal/actions.ts
✅ Files skipped from review due to trivial changes (1)
- packages/base/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/host/src/context.ts
- packages/host/src/internal/actions.ts
- packages/base/src/module-api/base.ts
jswalden
force-pushed
the
action-callback-return-result
branch
from
8bfaedc to
c868739
Compare
Julusian
force-pushed
the
action-callback-return-result
branch
3 times, most recently
from
505b261 to
70772cf
Compare
|
ok, Im happy enough with this to merge it. I am not keen on how much the I have pushed 70772cf here which I don't think breaks anything, and would be nice to keep that place simpler. I think using these stronger typings could already be a bit overwhelming, and presenting users with that as a starting point for their stronger typings will probably really confuse some |
jswalden
force-pushed
the
action-callback-return-result
branch
from
70772cf to
848be40
Compare
You could add an
This breaks a module's instance types that want to describe an action that has a result. I updated everything to rebase it onto main and #225. (I also had to force module version to |
jswalden
force-pushed
the
action-callback-return-result
branch
from
848be40 to
64abb22
Compare
jswalden
force-pushed
the
action-callback-return-result
branch
from
64abb22 to
cf25c4f
Compare
|
I rebased against main and added a second rev that undoes the API-breaking changes previously added under the theory that API could be broken while only a very few modules had updated to the 2.0.0 API. My sense is that, after a month and a half and however many more modules updating to 2.0.0, we can't quite swing that any more. If my sense is wrong, it's easy to accept only the first rev and discard the second. |
…reaking API change." This reverts commit cf25c4f.
|
Im not worried about that slightly breaking change, its still only 23 modules potentially affected, and its a pretty trivial change to adopt. I have been wondering if I should correct that in a patch of 2.0, but I'm not sure if that is a good idea |
2 participants
This is the
@companion-module/{base,host}side of an implementation of acontext.setResult(value)function, added for use in action callbacks, where the supplied value is then propagated into the next action in sequence as$(this:result).It might further be worth adding to
CompanionActionDefinitionan "exemplar" result value. Button and expression previews obviously can't know what a preceding action actually returned. It might be useful to have a sample value that can be fed into those preview mechanisms, in order to get somewhat better preview behavior than you get when$(this:result)is not the structured value you expect it to be. But I did not implement this.The only way I could figure out how to test this locally, in Companion, was to hard-code in
file:module versions that mean this isn't immediately landable. Those would need to change for this to be landed.Summary by CodeRabbit
New Features
Bug Fixes