Conversation
save_trial_parameters only accepted names found in the plugin's info object. Parameters that every plugin accepts, like css_classes and post_trial_gap, were rejected with a "Non-existent parameter" warning and never ended up in the data, even though the docs example saves post_trial_gap this way. Universal parameters are now resolved the same way the trial uses them: timeline variables and functions are evaluated, callbacks are saved as strings, an unset post_trial_gap falls back to default_iti, and data is saved with its properties evaluated. Setting one of them to false removes it from the data. Fixes jspsych#3501
🦋 Changeset detectedLatest commit: 174e146 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📦 Preview build readyBuilt from PR head Changed packages: Quick-start HTML: <script src="https://cdn.jsdelivr.net/gh/jspsych/jsPsych@8b5c740ba7e6a6fbb55ac717a9579b2bfbe7c3ba/packages/jspsych/dist/index.browser.min.js"></script>
<link rel="stylesheet" href="https://cdn.jsdelivr.net/gh/jspsych/jsPsych@8b5c740ba7e6a6fbb55ac717a9579b2bfbe7c3ba/packages/jspsych/css/jspsych.css">
<script src="https://cdn.jsdelivr.net/gh/jspsych/jsPsych@8b5c740ba7e6a6fbb55ac717a9579b2bfbe7c3ba/packages/plugin-html-keyboard-response/dist/index.browser.min.js"></script>All package URLs
Last updated 2026-09-26 21:02 UTC for PR head |
Member
|
Thanks for flagging this @rmz-oz and for proposing a fix. I think I've missed this while rebuilding the core library a couple of years ago. |
Instead of looking up universal parameters separately when saving the data, processParameters now evaluates them and adds them to trialObject, so plugins can access them too. save_trial_parameters reads them from trialObject like any other parameter, and the "Non-existent parameter" warning stays for names that are neither plugin nor universal parameters. An unset post_trial_gap gets the default_iti value. The data parameter is left as it was, since its properties are evaluated when the result is created.
Author
|
Thanks for the review! I've updated the PR as you suggested. Universal parameters are now evaluated in One thing I'd like your opinion on: a function passed to I've also added a note about using Claude Code to the description. Thanks for pointing that out. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
save_trial_parametersonly looked at the plugin's owninfo.parameters, so universal parameters (css_classes,post_trial_gap,on_start, ...) were rejected withNon-existent parameter "post_trial_gap" specified in save_trial_parameters.and left out of the data, although the example in docs/overview/plugins.md savespost_trial_gapthis way.Following @bjoluc's review, universal parameters are now evaluated in
processParametersand added totrialObject:FUNCTION, and missing values get their default. An unsetpost_trial_gapgetsdefault_iti, since that is the gap that is actually used.save_trial_parametersreads them fromtrialObjectlike plugin parameters. The warning is still shown for names that are neither plugin nor universal parameters.datais left out on purpose. Its properties are still evaluated when the result is created.One behavior change: a function passed to
post_trial_gapused to be called after the trial finished. Now it is called before the trial starts, together with the other dynamic parameters. This matches docs/overview/dynamic-parameters.md, but I can keep the old timing if you prefer.Tests:
Trial.spec.ts: plugins receive the evaluated universal parameters, they can be saved and removed withsave_trial_parameterswithout a warning, and thedefault_itifallback works. Four existing tests that compare the whole trial object now include the default universal values.tests/data/trialparameters.test.tswithhtml-keyboard-response, based on the case from the issue.Trial.ts, 8 fail. With the change,npx jest --cigives 739 passed, 6 skipped, 80 suites.npm run tscandnpm run buildpass (59/59 tasks each).AI use: I used Claude Code to write the code and the tests. I picked the issue, reproduced it, and reviewed and ran the changes myself.
Includes a patch changeset for
jspsychand adds me to contributors.md.Fixes #3501
🤖 Generated with Claude Code