Respect dynamically added trial/timeline descriptions #3426
Conversation
🦋 Changeset detectedLatest commit: 8665717 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 |
… node before original first node is done executing
…ic node add/remove
| expect(timeline.children.length).toEqual(2); | ||
| }); | ||
|
|
||
| it("respects dynamically added child node descriptions at the start", async () => { |
There was a problem hiding this comment.
I'm not sure this test makes sense. I think it will run the same exact trial twice, because of the runtime insertion stuff we looked at before.
There was a problem hiding this comment.
Yea I just tested it and you're right. should i just delete it?
There was a problem hiding this comment.
Although the additional tests might have been helpful to find out how this behaves, I think they all boil down to testing runtime behavior of the list iteration – and are rather complicated for that. Technically, the "respects dynamically added child node descriptions" case should be all it needs to check that the Timeline class dynamically creates trials one by one – the rest is up to the runtime. I'd prefer to keep the unit tests as slim as possible here (without sacrificing coverage). Sorry for involving into this again 🙊
Prior to this, trials (or nested timelines) that were added to a timeline description while the experiment was running would be ignored. This PR makes
Timelineobjects instantiate their child nodes incrementally before running them, thus accounting for dynamically added/removed trials and timelines.Closes #3379.
While I'm confident in the implementation, I'm submitting this as a draft for now because it lacks docs updates and a changeset (for which I don't have time ATM). @jspsych/core Feel free to complete this and release it!
I'm also not sure to which extent modifications to a JS array are reflected while iterating over it in a
for ... ofloop – it would be interesting to toy around with this a bit. Pushing elements to the end obviously works (and is the primary use case anyway), but what about removing elements prior to the current one? Index-based iteration would skip an element then – but maybefor ... ofdoesn't?