Skip to content

Commit

Permalink
fix: stack overflow when reloading existing recursive ViewModel with …
Browse files Browse the repository at this point in the history
…recursive input
  • Loading branch information
ascott18 committed Jun 10, 2024
1 parent eb9db37 commit 34f5285
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 2 deletions.
19 changes: 19 additions & 0 deletions src/coalesce-vue/src/viewmodel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1629,6 +1629,20 @@ export class ViewModelFactory {
this.map.set(initialData, vm);
}

private processed = new Map<any, Set<any>>();
/** Determine if the given initialData was already mapped to `vm`. */
shouldProcess(initialData: any, vm: ViewModel) {
let targets = this.processed.get(initialData);
if (!targets) {
this.processed.set(initialData, (targets = new Set()));
}
if (targets.has(vm)) {
return false;
}
targets.add(vm);
return true;
}

public static scope<TRet>(
action: (factory: ViewModelFactory) => TRet,
isCleanData: DataFreshness
Expand Down Expand Up @@ -2253,6 +2267,11 @@ export function updateViewModelFromModel<
skipStale = false
) {
ViewModelFactory.scope(function (factory) {
// Skip if we already applied this source to the target.
if (!factory.shouldProcess(source, target)) {
return;
}

// Add the root ViewModel to the factory
// so that when existing ViewModels are being updated,
// duplicate VM instances won't be created needlessly.
Expand Down
30 changes: 28 additions & 2 deletions src/coalesce-vue/test/viewmodel.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ describe("ViewModel", () => {

const parent = new CompanyViewModel();
parent.$loadCleanData({ companyId: 1, name: "existing parent" });
parent.employees;

// Act
const newChild = new PersonViewModel({ firstName: "bob" });
parent.employees?.push(newChild);
Expand Down Expand Up @@ -2552,7 +2552,7 @@ describe("ViewModel", () => {
expect(watchCallback).toBeCalledTimes(0);
});

test("doesnt stackoverflow on recursive object structures", () => {
test("doesnt stackoverflow when creating new recursive object structures", () => {
var studentModel = new Student({
studentId: 1,
studentAdvisorId: 1,
Expand All @@ -2572,6 +2572,32 @@ describe("ViewModel", () => {
expect(student).toBe(student.advisor!.students[0]);
});

test("doesnt stackoverflow when updating existing recursive object structures", () => {
var studentModel = new Student({
studentId: 1,
studentAdvisorId: 1,
advisor: { name: "Seagull", advisorId: 1 },
});
studentModel.advisor!.students = [studentModel];

const student = new StudentViewModel();
student.$loadCleanData(studentModel);

// Act
// Now that the ViewModel structure exists, try to update it.
student.$loadCleanData(studentModel);

// Assert
// First expectation: We made it this far without stackoverflowing.

// Second, different ways of traversing to the same VM should result in the same reference.
expect(student.advisor).toBe(student.advisor!.students[0].advisor);

// The root VM (`student`) should also be subject to this logic,
// so the root should be the same instance seen in the advisor's students array.
expect(student).toBe(student.advisor!.students[0]);
});

test("child object is reactive", async () => {
var course = new CourseViewModel({
student: { studentId: 1, name: "Steve" },
Expand Down

0 comments on commit 34f5285

Please sign in to comment.