[WIP] [Actions] Initial Action helper functions design

[Matthiee]

Action Helper Functions

An initial design. We are looking for general feedback on this.

See ActionHelpers.ts for all the functions.

Goal

Compared to using the more verbose studio.*-methods, have easy-to-use functions inside Actions JS.

VariableValue considerations

There are plenty of use cases where the value of a variable will be used as an argument for the next method. To make this easier you'll notice that VariableValue was added as an argument to almost all setter methods. This makes designing actions easier.

❌ Incorrect types

studio.layouts.select(studio.variable.getValue("myLayoutsListVariable")); // Argument of type 'VariableValue' is not assignable > to parameter of type 'string'.

✅ Runtime type check

selectLayout(studio.variable.getValue("myLayoutsListVariable")); // Works

Entities with a name can be used as name argument

In order to avoid having to use entity.name everywhere, you'll notice IHasName was added to the argument list.

byName is an auto-complete action, that already contains the name.

❌ Explicit .name

studio.variables.setValue(studio.variable.byName("myVariable").name, "some value"); // manual .name is needed

✅ Runtime name lookup

setVariableValue(studio.variable.byName("myVariable"), "some value");

Related tickets

• EDT-977

[github-actions]

Coverage report can be checked at https://chili-dev.azureedge.net/sdk/coverage/328/coverage.html

[github-actions]

Unit Test Results

    1 files    31 suites   25s ⏱️
277 tests 277 ✔️ 0 💤 0 ❌
278 runs  278 ✔️ 0 💤 0 ❌

Results for commit eb70b3a.

♻️ This comment has been updated with latest results.

[seancrowe]

I'll continue reviewing, but two initial points:

1. Error Messaging: Enhancing error messaging is crucial. It's beneficial for clients to clearly understand their mistakes. I am happy to see that we are implementing a more friendly message. I did not review what was done here too deeply beyond a recognition.

2. Function Design: Some functions, like getVariable and getTriggeredVariable, returning objects could be perplexing, for non-programmers. While this might simplify property access for those with a little developer experience (and the seasonal developers too), it assumes a prior understanding of objects. Which in my experience, objects are not intuitive to users who have no experience.

For clarity, instead of a single getFrame function returning a frame object, consider separate functions for each frame property.

To gauge which approach is more intuitive, I'll consult non-programmers on their preference:

• Accessing properties like: getFrame("myFrame").x

• Direct value retrieval: getFrameX("myFrame")

My inclination is they'll prefer the latter.

Additionally, I'm interested in their expectations from getVariable. My guess? They'd anticipate the return of the variable's value, not an object.

I'm also curious about their expectations from getFrame. If objects are not intuitive, what would they expect here? My guess is they will expect an object - but not one with methods.

To clarify, it's not strictly an either-or decision between these methodologies. We could potentially offer both. However, I suggest introducing more direct helper functions and not assuming everyone will be comfortable with object property retrieval.

I will update this PR with the results of my non-programmer friends.

[seancrowe]

Results

@Matthiee I got my results, and as predicted the non-programmers assumed getVairable was the same as getVariableValue().

I explained that it returns a variable object, but they only kind of understood. They preferred getVariableValue("name") over getVariable("name").value.

For getFrame, they were even more confused and would say "a frame", but when I asked them what that meant they did not know.

So we could keep these functions, but I wouldn't go deeply on making functions that return objects.

Arguments

Becareful to name arguments to help users, and are consistent. A lot of functions have short names like this one:

setVariableValue(variable: string | Variable, value?: VariableValue)

It should be

setVariableValue(name: string | Variable, value?: VariableValue)

Further Help

I strongly suggest we make sure to add helper functions that would cover the use cases clients would want, and review from their perspective. If you would like, I can make a PR to this branch of things I would add or change.

[Matthiee]

Great feedback @seancrowe.

I will discuss this with the team and change the design where needed. If you have time for a PR with additional changes that would be very helpful.

[jeroencosyn]

@Matthiee, in your comment this is indicated with a ❌:
studio.variables.setValue(studio.variable.byName("myVariable").name,` "some value"); // manual .name is needed

But this is valid, no? There's just an easier way to do it now with the new helper function?

[Matthiee]

@Matthiee, in your comment this is indicated with a ❌: studio.variables.setValue(studio.variable.byName("myVariable").name,` "some value"); // manual .name is needed

But this is valid, no? There's just an easier way to do it now with the new helper function?

@jeroencosyn correct, that is valid right now. The main concern here is that when you do studio.variables. and auto-complete the action you get the byName("my variable"). However, that auto-complete action is not valid in this context and you must type .name at the end, which may not be evident for non-technical designers.

The suggestion I provided here is that the helper functions would allow the variable object returned by the byName to be a valid argument for the setVariableValue helper function from a TypeScript perspective. So it would no longer show up as a type error in the code editor.

[seancrowe]

There has been internal discussions about adding a bit more type safety, mostly to avoid unwanted behaviors and runtime errors.

If we go with adding type safety to Helper Functions, we need to remove VariableValue (or an any type) from helper functions to avoid any extra undefined behavior or errors at runtime due to type-mismatch.

Quick note on passing in Variable instead of a name as string. If we consider Variable like putting in a wrong variable name, which I think is the same risk of the same runtime error, then it would be acceptable to pass in the "I don't know" Variable. The counterargument is that when you pass in a string, it is clear to the use that the "variable" may not exist while passing in Variable makes it appear as it does exist and is right - meaning you may forget that type error could happen.

The idea would to add a type safe barrier to getting and setting variables by requiring users to define the variable type via helper functions.

Getting and setting a boolean variable type signature

getBooleanVariableValue(name:string) => boolean
setBooleanVariableValue(name:string, value:boolean) => undefined

of if we allow Variable

getBooleanVariableValue(name:string|Variable) => boolean
setBooleanVariableValue(name:string|Variable, value:boolean) => undefined

If the user passes in the name of a missing variable or the value of a variable that is not a boolean, they get a helpful error message. All these functions should behave similar.

For setting text variables, we can be more excepting because one would expect a number passed in to be turned into a string of that number

getTextVariableValue(name:string) => string
setTextVariableValue(name:string, value:string|number) => undefined

or if we allow Variable

getTextVariableValue(name:string|Variable) => string
setTextVariableValue(name:string|Variable, value:string|number) => undefined

For list, we already have a number of functions defined for list variables.

For images, that is a problem. Right now, image variables return a Proxy, where from my testing only get works and set does not work. I currently cannot give advice because I don't know how image variables are supposed to behave in actions and how they will work with media connectors. That needs to be defined.

---

What about copyVariableValue?

The function I think would have the same type signature, the only difference here is that it will error out if the variable types are not compatible. We should be explicit about this in the documentation.

We could potentially change the naming to be more explicit about the behavior, but I don't have any good ideas: copyVariableValueIfCompatibleTypes lol 🤣

---

What about triggers.changedVariable?

This is where all this work comes feels like it comes to a crash because changedVariable really is of type Variable. There is no way around that without separated triggers types.

The good news is that I think most users will trigger a specific variable (or variables). If an end user is using a trigger on any variable, they should already be above the beginner level action user. The only thing you can do is add helper functions for type checking.

Maybe add a helper `isVariableOfType(name:string|Variable, variableType:VariableType)

var name = getTriggeredVariableName();
if (isVariableOfType(name, VariableType.Text)) {
  
}

You instead go the route of adding a checking function for each variable type:

var name = getTriggeredVariableName();
if (isVariableTypeText(name)) {
  
}

if (isVariableTypeImage(name)) {
  
}

if (isVariableTypeSingleList(name)) {
  
}

if (isVariableTypeBoolean(name)) {
  
}

However, again I suspect isVariableOfType is good enough because we are talking about people beyond beginner at this point.

[Matthiee]

Status update

Changes

• Now that [Feature] [EDT-1086] Improved get/set for Actions #331 is merged I was able to reduce the helper functions implementation code significantly.
• I addressed most of the PR comments

Todo

• Review and update comments per helper function
• Make final decision regarding VariableValue

[pkgacek]

@Matthiee what is the status of this PR?

[Matthiee]

@pkgacek I'm currently making some final changes in the scope of EDT-1155 to the Action Types and Helper functions (should be done today).

After that, it depends on whether #351 was already merged, as the helper file in this PR is in the wrong place.

Some changes might need to be made to this file regarding WRS-1343.

[Matthiee]

Closing this PR as work for Action Helper Functions will continue in #354