fix(journal): re-publish only changed declarations
topoSortAffected followed declared dependencies even when they were not in the affected set, so reevaluateDependents re-evaluated and re-published variables that had not changed. This could also overwrite a value just set via /set with the declaration's own expression.
This commit is contained in:
@@ -109,14 +109,16 @@ The items below are still open.
|
||||
(#1) for declarations/modifiers and through `dispatchError` for `/set` and
|
||||
`/roll`.
|
||||
|
||||
### 7. `topoSortAffected` re-evaluates untouched declarations — low
|
||||
### 7. `topoSortAffected` re-evaluates untouched declarations — ✅ fixed
|
||||
|
||||
- **Where:** in `var-reactivity.ts`, the visit condition
|
||||
`affected.has(dep) || declExprs.has(dep)` pulls declared dependencies that are
|
||||
not in `affected` into the result.
|
||||
- **Symptom:** `reevaluateDependents` re-evaluates and re-publishes variables
|
||||
that did not change. Mostly harmless, but noisy.
|
||||
- **Fix:** only visit/topologically order keys in `affected`.
|
||||
that did not change. Mostly harmless, but noisy — and a re-evaluated
|
||||
declaration could overwrite a value that had just been `/set`.
|
||||
- **Fixed:** the visit condition now only follows dependencies in `affected`,
|
||||
so a cascade re-evaluates exactly the changed variable's dependents.
|
||||
|
||||
### 8. Failed `initReactivity` leaves partial state — ✅ fixed
|
||||
|
||||
|
||||
@@ -844,6 +844,25 @@ describe("var-reactivity", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeCascade — declarations", () => {
|
||||
test("does not re-publish or re-evaluate declared dependencies that did not change", () => {
|
||||
initReactivity({
|
||||
declarations: [
|
||||
{ key: "$con", expression: "12" },
|
||||
{ key: "$hp", expression: "$con * 5" },
|
||||
],
|
||||
tagModifiers: [],
|
||||
});
|
||||
|
||||
// $con was set to 14; only its dependent $hp should be re-evaluated.
|
||||
setBase("$con", "14");
|
||||
const cascade = computeCascade("$con", "12", store({ $con: "14" }));
|
||||
|
||||
expect(cascade.map((r) => r.key)).toEqual(["$hp"]);
|
||||
expect(cascade[0].value).toBe("70");
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeCascade — tag activation/deactivation", () => {
|
||||
test("activates tag modifiers when a tag value is set", () => {
|
||||
initReactivity({
|
||||
|
||||
@@ -797,7 +797,10 @@ function topoSortAffected(affected: Set<string>): string[] {
|
||||
if (expr) {
|
||||
const deps = extractDependencies(expr);
|
||||
for (const dep of deps) {
|
||||
if (affected.has(dep) || declExprs!.has(dep)) {
|
||||
// Only order within the affected set. Declared dependencies that did
|
||||
// not change must not be pulled in, or reevaluateDependents would
|
||||
// re-publish variables that were never touched.
|
||||
if (affected.has(dep)) {
|
||||
visit(dep);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user