diff --git a/packages/blaze/builtins.js b/packages/blaze/builtins.js index dc7525e04..0fd037dda 100644 --- a/packages/blaze/builtins.js +++ b/packages/blaze/builtins.js @@ -217,6 +217,9 @@ Blaze.Each = function (argFunc, contentFunc, elseFunc) { eachView.elseFunc = elseFunc; eachView.argVar = undefined; eachView.variableName = null; + // Fired by `afterDiff` to revive item view renders that were deferred + // while this each view was pending a sequence update. See meteor/blaze#468. + eachView._eachItemPendingDep = new Tracker.Dependency(); // update the @index value in the scope of all subviews in the range const updateIndices = function (from, to) { @@ -250,6 +253,19 @@ Blaze.Each = function (argFunc, contentFunc, elseFunc) { eachView.stopHandle = ObserveSequence.observe(function () { return eachView.argVar.get()?.value; }, { + // Called immediately when the sequence source is invalidated, + // BEFORE the Tracker flush re-runs other autoruns. This freezes + // item views so their helpers don't re-run with stale data. + // See meteor/blaze#468. + onInvalidate: function () { + if (!eachView._domrange) return; + const members = eachView._domrange.members; + for (let i = 0; i < members.length; i++) { + if (members[i] && members[i].view) { + members[i].view._eachItemPendingUpdate = eachView._eachItemPendingDep; + } + } + }, addedAt: function (id, item, index) { Tracker.nonreactive(function () { let newItemView; @@ -340,6 +356,20 @@ Blaze.Each = function (argFunc, contentFunc, elseFunc) { subviews.splice(toIndex, 0, itemView); } }); + }, + // Called after the diff is applied. Clear the pending flag on + // surviving item views, then fire the revival dependency so any item + // render that was deferred during the update re-runs with fresh data. + afterDiff: function () { + if (eachView._domrange) { + const members = eachView._domrange.members; + for (let i = 0; i < members.length; i++) { + if (members[i] && members[i].view) { + delete members[i].view._eachItemPendingUpdate; + } + } + } + eachView._eachItemPendingDep.changed(); } }); diff --git a/packages/blaze/view.js b/packages/blaze/view.js index ff5547e7f..14c582c34 100644 --- a/packages/blaze/view.js +++ b/packages/blaze/view.js @@ -471,6 +471,28 @@ Blaze._materializeView = function (view, parentView, _workStack, _intoArray) { Tracker.nonreactive(function () { view.autorun(function doRender(c) { // `view.autorun` sets the current view. + + // Skip re-render if this view or an ancestor is an #each item that + // is pending a sequence update. This prevents stale renders where an + // item's helpers re-run before ObserveSequence has had a chance to + // update or remove it. Instead of returning silently — which would + // leave this computation with zero reactive dependencies and thus + // permanently dead — we depend on the owning each view's revival + // dependency. ObserveSequence's `afterDiff` fires that dependency + // once the pending flag is cleared, so this computation re-runs and + // re-collects its real dependencies with fresh data. See + // meteor/blaze#468. + if (!c.firstRun) { + let v = view; + while (v) { + if (v._eachItemPendingUpdate) { + v._eachItemPendingUpdate.depend(); + return; + } + v = v.parentView; + } + } + view.renderCount = view.renderCount + 1; view._isInRender = true; // Any dependencies that should invalidate this Computation come diff --git a/packages/observe-sequence/observe_sequence.js b/packages/observe-sequence/observe_sequence.js index a18ce2acf..15f97166d 100644 --- a/packages/observe-sequence/observe_sequence.js +++ b/packages/observe-sequence/observe_sequence.js @@ -112,9 +112,16 @@ ObserveSequence = { // general 'key' argument which could be a function, a dotted // field name, or the special @index value. let lastSeqArray = []; // elements are objects of form {_id, item} - const computation = Tracker.autorun(function () { + const computation = Tracker.autorun(function (c) { const seq = sequenceFunc(); + // When this computation is invalidated (sequence source changed), + // immediately notify callers so they can freeze item views BEFORE + // the flush re-runs other autoruns. See meteor/blaze#468. + if (callbacks.onInvalidate) { + c.onInvalidate(() => callbacks.onInvalidate()); + } + Tracker.nonreactive(function () { let seqArray; // same structure as `lastSeqArray` above. @@ -142,9 +149,27 @@ ObserveSequence = { throw badSequenceError(seq); } - diffArray(lastSeqArray, seqArray, callbacks); - lastSeq = seq; - lastSeqArray = seqArray; + // Allow callers to prepare for the diff (e.g., freeze item views + // that are about to be removed). See meteor/blaze#468. + if (callbacks.beforeDiff) { + callbacks.beforeDiff(lastSeqArray, seqArray); + } + + try { + diffArray(lastSeqArray, seqArray, callbacks); + + // Only record the new baseline once the diff has fully applied. + // If a diff callback throws, the DOM no longer matches seqArray, + // so the last consistent baseline is kept for the next diff. + lastSeq = seq; + lastSeqArray = seqArray; + } finally { + // Always run afterDiff, even if a diff callback threw, so item + // views are never left permanently frozen. See meteor/blaze#468. + if (callbacks.afterDiff) { + callbacks.afterDiff(); + } + } }); }); diff --git a/packages/observe-sequence/observe_sequence_tests.js b/packages/observe-sequence/observe_sequence_tests.js index 01c471922..25e35808c 100644 --- a/packages/observe-sequence/observe_sequence_tests.js +++ b/packages/observe-sequence/observe_sequence_tests.js @@ -742,3 +742,48 @@ Tinytest.addAsync('observe-sequence - cursor to other cursor, same collection', ]); }); +// #468 — `afterDiff` must always run, even if a diff callback (e.g. an +// item view's first render in Blaze.Each) throws mid-diff. Otherwise +// callers that freeze state in `onInvalidate`/`beforeDiff` and release it +// in `afterDiff` would be left permanently frozen by a transient error. +Tinytest.add( + 'observe-sequence - afterDiff runs even when a diff callback throws', + function (test) { + const dep = new Tracker.Dependency(); + let seq = [{ _id: '1' }]; + let afterDiffCount = 0; + + const handle = ObserveSequence.observe(function () { + dep.depend(); + return seq; + }, { + addedAt: function (id) { + if (id === '2') { + throw new Error('intentional failure while adding item 2'); + } + }, + afterDiff: function () { + afterDiffCount++; + }, + }); + + // Initial diff added item '1' and ran afterDiff once. + test.equal(afterDiffCount, 1); + + // Re-run with a throwing addedAt. The error surfaces on the re-run + // (logged or rethrown by the flush); either way afterDiff must run. + seq = [{ _id: '1' }, { _id: '2' }]; + dep.changed(); + try { + Tracker.flush(); + } catch (e) { + // Expected: the thrown diff callback may surface here. + } + + test.equal(afterDiffCount, 2, + 'afterDiff did not run after a diff callback threw'); + + handle.stop(); + } +); + diff --git a/packages/spacebars-tests/template_tests.html b/packages/spacebars-tests/template_tests.html index 34322ade9..7f29ed409 100644 --- a/packages/spacebars-tests/template_tests.html +++ b/packages/spacebars-tests/template_tests.html @@ -1172,3 +1172,31 @@
{{item}}
{{/each}} + + + + + + + + + + diff --git a/packages/spacebars-tests/template_tests.js b/packages/spacebars-tests/template_tests.js index 2821daf54..7be627cf5 100644 --- a/packages/spacebars-tests/template_tests.js +++ b/packages/spacebars-tests/template_tests.js @@ -4480,3 +4480,175 @@ Tinytest.add( ); } ); + +// #468 — #each stale data context +// When the parent data context changes and the #each sequence returns +// different items, item views should NOT re-render with stale data +// before being removed. +Tinytest.add( + 'spacebars-tests - template_tests - #each no stale render on different IDs', + function (test) { + const parentTmpl = Template.spacebars_template_test_each_stale_parent1; + const childTmpl = Template.spacebars_template_test_each_stale_child1; + + const mode = new ReactiveVar('foo'); + const renderLog = []; + + parentTmpl.helpers({ + mode: function () { return mode.get(); }, + }); + + childTmpl.helpers({ + getItems: function () { + const foo = Template.currentData().foo; + if (foo === 'foo') { + return [{ _id: '1', msg: 'foo-item' }]; + } + return [{ _id: '2', msg: 'bar-item' }]; + }, + logRender: function (msg) { + const dataFoo = Template.instance().data.foo; + renderLog.push({ msg, dataFoo }); + return ''; + }, + }); + + const div = renderToDiv(parentTmpl); + + // Initial render + test.equal(renderLog.length, 1); + test.equal(renderLog[0].msg, 'foo-item'); + test.equal(renderLog[0].dataFoo, 'foo'); + + // Switch — should NOT produce a stale render where msg="foo-item" with dataFoo="bar" + renderLog.length = 0; + mode.set('bar'); + Tracker.flush(); + + // Every render should have consistent msg and dataFoo + renderLog.forEach(function (entry) { + if (entry.msg === 'foo-item') { + test.equal(entry.dataFoo, 'foo', 'stale: foo-item rendered with dataFoo=bar'); + } + if (entry.msg === 'bar-item') { + test.equal(entry.dataFoo, 'bar', 'stale: bar-item rendered with dataFoo=foo'); + } + }); + + // Final render should be bar-item + const last = renderLog[renderLog.length - 1]; + test.equal(last.msg, 'bar-item'); + test.equal(last.dataFoo, 'bar'); + } +); + + +Tinytest.add( + 'spacebars-tests - template_tests - #each no stale render with each-in syntax', + function (test) { + const parentTmpl = Template.spacebars_template_test_each_stale_parent3; + const childTmpl = Template.spacebars_template_test_each_stale_child3; + + const mode = new ReactiveVar('foo'); + const renderLog = []; + + parentTmpl.helpers({ + mode: function () { return mode.get(); }, + }); + + childTmpl.helpers({ + getItems: function () { + const foo = Template.currentData().foo; + if (foo === 'foo') { + return [{ _id: '1', msg: 'foo-item' }]; + } + return [{ _id: '2', msg: 'bar-item' }]; + }, + logRenderItem: function (item) { + const dataFoo = Template.instance().data.foo; + renderLog.push({ msg: item.msg, dataFoo }); + return ''; + }, + }); + + const div = renderToDiv(parentTmpl); + renderLog.length = 0; + mode.set('bar'); + Tracker.flush(); + + renderLog.forEach(function (entry) { + if (entry.msg === 'foo-item') { + test.equal(entry.dataFoo, 'foo', 'stale: foo-item rendered with dataFoo=bar'); + } + }); + + const last = renderLog[renderLog.length - 1]; + test.equal(last.msg, 'bar-item'); + test.equal(last.dataFoo, 'bar'); + } +); + + +// #468 (parallel path) — a SURVIVING item view (same _id, changed inner +// data) must still re-render to its new data after the sequence update, +// and must never render a stale (msg, dataFoo) pair in between. This +// guards against the freeze-on-pending logic stranding views that are +// kept (changedAt) rather than removed. +Tinytest.add( + 'spacebars-tests - template_tests - #each surviving item re-renders after sequence update', + function (test) { + const parentTmpl = Template.spacebars_template_test_each_stale_parent2; + const childTmpl = Template.spacebars_template_test_each_stale_child2; + + const mode = new ReactiveVar('foo'); + const renderLog = []; + + parentTmpl.helpers({ + mode: function () { return mode.get(); }, + }); + + childTmpl.helpers({ + // Same _id across the flip => ObserveSequence reports changedAt + // (the item survives) rather than removedAt/addedAt. + getItems: function () { + const foo = Template.currentData().foo; + return [{ _id: '1', msg: foo === 'foo' ? 'foo-msg' : 'bar-msg' }]; + }, + logRender: function (msg) { + const dataFoo = Template.instance().data.foo; + renderLog.push({ msg, dataFoo }); + return ''; + }, + }); + + const div = renderToDiv(parentTmpl); + + test.equal(renderLog.length, 1); + test.equal(renderLog[0].msg, 'foo-msg'); + test.equal(renderLog[0].dataFoo, 'foo'); + test.matches(canonicalizeHtml(div.innerHTML), /foo-msg/); + + renderLog.length = 0; + mode.set('bar'); + Tracker.flush(); + + // No stale pairing during the transition. + renderLog.forEach(function (entry) { + if (entry.msg === 'foo-msg') { + test.equal(entry.dataFoo, 'foo', 'stale: foo-msg rendered with dataFoo=bar'); + } + if (entry.msg === 'bar-msg') { + test.equal(entry.dataFoo, 'bar', 'stale: bar-msg rendered with dataFoo=foo'); + } + }); + + // The surviving view must have re-rendered to the new data — both in + // the render log and in the live DOM (catches a frozen/stuck view). + const last = renderLog[renderLog.length - 1]; + test.equal(last.msg, 'bar-msg'); + test.equal(last.dataFoo, 'bar'); + test.matches(canonicalizeHtml(div.innerHTML), /bar-msg/); + test.equal(/foo-msg/.test(canonicalizeHtml(div.innerHTML)), false, + 'stale DOM: surviving view still shows old data after update'); + } +);