Custom JS calls#1082
Conversation
| inputs: Array<number>; | ||
| outputs: Array<number>; | ||
| queue: boolean; | ||
| js: string | null; |
There was a problem hiding this comment.
Should this be string | false?
|
Very nice. I tested it and it works well. Just left a few small comments above |
pngwn
left a comment
There was a problem hiding this comment.
This is looking great.
In addition to the comment I left about evaluating the code when processing the config and adjusting the implementation, this implementation has one important omission: the ability to update state.
The function needs to be passed the data from the input components, as it currently is, but it should also be passed the current state of the oputput components. In addition, the return value (if there is one) should become the new state for the output components.
So to clarify, i think the following needs to happen:
- function has two parameters
- The value of the input components (same as API payload)
- The current state of the output components (as single item or array, depending on quantity)
- If there is a return value then that should be used to set the state of the output components immediately.
- If there is no return value then no local state will be changed. This means we will not be able to assign an undefined value to a single output but i think that is a reasonabel tradeoff.
Something like this:
// input is a single textbox, output is a chatbot
function js(data, state) {
return [...state, [data, ""]]
}This would immediately update the state of the chatbot
In python we'd then do something like tail(history).push(repsonse) rather than adding both the message and the response.
Question I have regarding state updates is what happens if the returned value doesn't come in the expected shape. Say we have a list iof outputs but the returned value is a single item. Or what if the list of outputs and the list of return values is not equal? We could technically validate some of these values too if needed but i think we should do that later, if we decide it makes sense.
| if (js !== false) { | ||
| console.log(js); | ||
| try { | ||
| var jsfn = eval(js); |
There was a problem hiding this comment.
We should do this up front when we are parsing the config rather than doing ti everytime we make a call. The sooner we can treat this as a normal function the easier our lives will be.
Additionally, new Function() is preferable to eval as it is faster and has safer scoping rules.
Something like this shoudl give the same result
const jsfn = new Function(`return ${js}`)();There was a problem hiding this comment.
Done. Instead of combining input data in one list called data and output data in one arg called state, I spread the args to match the style used in python - the output state values come right after input arguments.
|
Okay, I slightly misread this initially. This allows an alternative (client) implementation to the API request. I think this needs to be in addition to the API request. The point about the additional parameters remains true. When generating state for existing components, we need access to the current state as well, so that we can derive the new state. Can we currently do: |
@pngwn if we need the state of the output component for whatever reason, can't we simply include the output component as one of the input components? I don't think we don't need to pass in the state of the output as a special parameter
I agree that's probably a better design. But then we have to clarify whether the js function happens first and then the Python function takes in the updated values of the components, or however that should work etc. Also, right now a user could simply add two different event handlers: one for the Python function, and one for the js function, with the current design. |
We could do, i just think it is confusing and annoying. In my mind the main (only?) usecase for this is updating component state prior to or instead of an API request. Passing the current state just seems like a sensible convenience rather than making people add the output as an input (which will then require them to set the output component to Almost every state management mechanism passes both the event payload and the existing state by default because people typically need the ability to compute state based on the current state and the state of the payload. |
|
Another note, we need to ensure that any API calls are sent off before we manipulate state via JS otehrwise we could end up with nasty race conditions. This could be controllable via an option. |
|
All requested changes made @pngwn, take a look at blocks_js_methods/run.py for an example. |
Allows users to wrap JS functions in addition to regular python functions. "Clear" now uses this functionality.