jspsych / jsPsych

Create behavioral experiments in a browser using JavaScript
http://www.jspsych.org
MIT License
1.05k stars 679 forks source link

"array: true" plugin parameter throws error when jsPsych.timelineVariable() is used (audio-button-response plugin) #3412

Open devsda1 opened 1 month ago

devsda1 commented 1 month ago

While updating a task programmed in jsPsych 7.x to 8.x, I found that supplying the option "choices: jsPsych.timelineVariable('choices')" to the audio-button-response plugin now causes the following error:

A non-array value (function() { return fnGetImmediate(getArg); }) was provided for the array parameter "choices" in the "audio-button-response" plugin. Please make sure that "choices" is an array.

The cause of this seems to be the newly strict handling of "array: true" for plugin parameters. It's true - the value here will be a function, not an array.

But surely when a plugin sets "array: true", it should not break the use of timeline variables with that parameter?

bjoluc commented 1 month ago

Yup, fully agree – thanks @devsda1! I'll look into this on Thursday.

bjoluc commented 1 month ago

@devsda1 Timeline variable evaluation for array parameters works fine; your error message suggests that your timeline variable evaluates to a function instead of an array. Can you take a look at it again?

jodeleeuw commented 2 weeks ago

@devsda1 Just checking in to see if the source of the problem got tracked down?

devsda1 commented 2 weeks ago

@jodeleeuw @bjoluc My code that worked in 6.x/7.x populated certain timeline variables with function values in order to defer evaluation until the trial is about to run. The reason for that is to provide the correct content for the participant's language, which is selected only after the outer timeline starts running.

In 6.x/7.x, jsPsych helpfully called functions that were returned by timelineVariable() (i.e. by the function provided by timelineVariable).

In 8.x, it does not. So in this situation, rather than e.g. simply providing jsPsych.timelineVariable('choices') as the value of the 'choices' parameter, I need to do something like this:

function resolveFunctions(val) {
    if (typeof(val) === 'function') {
        return resolveFunctions(val());
    }
    if (typeof(val) === 'object') {
        // works for objects and arrays
        for (let name in val) {
            val[name] = resolveFunctions(val[name]);
        }
    }
    return val;
}

function resolveTimelineVarLater(name) {
    return function() {
        return resolveFunctions(jsPsych.evaluateTimelineVariable(name));
    }
}

// ...

timeline.push({
    // ...
    choices: resolveTimelineVarLater('choices'),
    // ...
});

I'm not sure that a value obtained by jsPsych from a timeline variable needs to be treated differently from one supplied as the parameter value itself, in terms of how functions are resolved to values and (if the plugin specifies a type constraint) the value types are ultimately checked. I didn't expect that to be the case, anyway. But if I need to have extra code like the above in my task to handle this, at least now I know how to do it.

bjoluc commented 2 weeks ago

While looking at the v7 code again, I noticed it was evaluating timeline variables first and functions afterwards. I can imagine that was done to avoid calling a timeline variable object's methods while recursively evaluating functions. v8 reverses that order.

I didn't notice I changed this behavior with v8. I remember actively deciding for the v8 evaluation order (I think I considered it more practical and intuitive to make functions return timeline variables than to use functions as timeline variable values – @jodeleeuw Curious to hear your opinion), but I think I didn't realize v7 was already evaluating functions returned from timeline variables.

As for your case @devsda1, providing a function that evaluates the timeline variable and any functions the variable might resolve to seems to be your best bet with v8 – sorry for this!

devsda1 commented 2 weeks ago

@bjoluc Thanks for the information! Not sure if I read the code correctly, but it looks like the new version also will only do one step of calling a function, whereas in 7.x replaceFunctionsWithValues is used, which recursively walks an object or array in a similar (but more jsPsych specific) way as my "resolveFunctions" code above?

A maximally magic/flexible approach could be for replaceFunctionsWithValues (or something similar) to also resolve timeline variables if they are encountered during the walk. Then, whatever is supplied as a parameter value, any contained functions or timeline variables will be replaced until all values are resolved. (However if you need to resolve timeline variables from a parent timeline, which I haven't used myself but seems to be supported, I can see how it would become more complex to implement; I guess you'd have to inject the parent timelines into any recursion?)

bjoluc commented 2 weeks ago

Not sure if I read the code correctly, but it looks like the new version also will only do one step of calling a function, whereas in 7.x replaceFunctionsWithValues is used, which recursively walks an object or array in a similar (but more jsPsych specific) way as my "resolveFunctions" code above?

The method is called recursively according to the plugin's parameter definitions. This means it no longer does any magic on recursive properties that are not listed in the plugin's parameter description (intended and noted in the v8 release notes) – something which you might previously have relied on as well.

A maximally magic/flexible approach

I'm afraid, we have decided against the full magic version on purpose to avoid calling functions when it is not intended (e.g., on third-party objects).

devsda1 commented 2 weeks ago

Ah I see, thanks. If I wanted to roll my own magic, I think I could just add a conditional evaluateTimelineVariable() in my recursive function.

Is the TimelineVariable class visible from the context of the script that is calling jsPsych? I was thinking of something like "if (val instanceof TimelineVariable) {...}".