Skip to content

Commit 4defbd2

Browse files
airhornsairhorns
committed
Ensure types.maybe(types.reference()) types are resolved late after construction
This fixes a bug in the fast instantiator code where valid references would throw a `can't resolve reference` error due to the instantiation order within MQT. We were accidentally eagerly resolving some references when constructing a tree, instead of late resolving them at the end like we normally do. This only happened for wrapped reference types like a `maybe(reference())` or a `late(reference())` because of an instanceof check instead of a deeper `isReferenceType` check. Fixes #78, which was an overly broad bug report as it turns out -- class models and normal models correctly late resolve references, regardless of property definition order before this change, it's just when you wrap the reference in a maybe that things got wonky. There's one important little catch as well, which is that unions of references are `isReferenceType()` in MST. We need to carefully handle these, and not assume all union elements are references if one is. For these rare but complicated cases, we fall back to the normal instantiation path that is a bit slower, but run it in a callback at the end of construction so that any reference members of the union are running at the right time. Woop woop!
1 parent db09494 commit 4defbd2

File tree

4 files changed

+486
-230
lines changed

4 files changed

+486
-230
lines changed

‎spec/class-model-references.spec.ts

Lines changed: 146 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
import { read } from "fs";
12
import type { IAnyClassModelType, InstanceWithoutSTNTypeForType, IReferenceType, StateTreeNode } from "../src";
23
import { Instance, types } from "../src";
34
import { action, ClassModel, register } from "../src/class-model";
5+
import { Apple } from "./fixtures/FruitAisle";
6+
import { NameExample } from "./fixtures/NameExample";
47
import { create } from "./helpers";
58

69
@register
@@ -37,132 +40,175 @@ class Root extends ClassModel({
3740
refs: types.array(Referrable),
3841
}) {}
3942

40-
describe("clas model references", () => {
41-
test("can resolve valid references", () => {
42-
const root = create(
43-
Root,
44-
{
45-
model: {
46-
ref: "item-a",
43+
describe("class model references", () => {
44+
describe.each([
45+
["read-only", true],
46+
["observable", false],
47+
])("%s", (_name, readOnly) => {
48+
test("can resolve valid references", () => {
49+
const root = create(
50+
Root,
51+
{
52+
model: {
53+
ref: "item-a",
54+
},
55+
refs: [
56+
{ key: "item-a", count: 12 },
57+
{ key: "item-b", count: 523 },
58+
],
4759
},
48-
refs: [
49-
{ key: "item-a", count: 12 },
50-
{ key: "item-b", count: 523 },
51-
],
52-
},
53-
true,
54-
);
60+
readOnly,
61+
);
5562

56-
expect(root.model.ref).toEqual(
57-
expect.objectContaining({
58-
key: "item-a",
59-
count: 12,
60-
}),
61-
);
62-
});
63+
expect(root.model.ref).toEqual(
64+
expect.objectContaining({
65+
key: "item-a",
66+
count: 12,
67+
}),
68+
);
69+
});
70+
71+
test("throws for invalid refs", () => {
72+
const createRoot = () => {
73+
const instance = create(
74+
Root,
75+
{
76+
model: {
77+
ref: "item-c",
78+
},
79+
refs: [
80+
{ key: "item-a", count: 12 },
81+
{ key: "item-b", count: 523 },
82+
],
83+
},
84+
readOnly,
85+
);
86+
instance.model.ref;
87+
};
6388

64-
test("throws for invalid refs", () => {
65-
const createRoot = () =>
66-
create(
89+
expect(createRoot).toThrow();
90+
});
91+
92+
test("can resolve valid safe references", () => {
93+
const root = create(
6794
Root,
6895
{
6996
model: {
70-
ref: "item-c",
97+
ref: "item-a",
98+
safeRef: "item-b",
7199
},
72100
refs: [
73101
{ key: "item-a", count: 12 },
74102
{ key: "item-b", count: 523 },
75103
],
76104
},
77-
true,
105+
readOnly,
78106
);
79107

80-
expect(createRoot).toThrow();
81-
});
108+
expect(root.model.safeRef).toEqual(
109+
expect.objectContaining({
110+
key: "item-b",
111+
count: 523,
112+
}),
113+
);
114+
});
82115

83-
test("can resolve valid safe references", () => {
84-
const root = create(
85-
Root,
86-
{
87-
model: {
88-
ref: "item-a",
89-
safeRef: "item-b",
116+
test("does not throw for invalid safe references", () => {
117+
const root = create(
118+
Root,
119+
{
120+
model: {
121+
ref: "item-a",
122+
safeRef: "item-c",
123+
},
124+
refs: [
125+
{ key: "item-a", count: 12 },
126+
{ key: "item-b", count: 523 },
127+
],
90128
},
91-
refs: [
92-
{ key: "item-a", count: 12 },
93-
{ key: "item-b", count: 523 },
94-
],
95-
},
96-
true,
97-
);
129+
readOnly,
130+
);
98131

99-
expect(root.model.safeRef).toEqual(
100-
expect.objectContaining({
101-
key: "item-b",
102-
count: 523,
103-
}),
104-
);
105-
});
132+
expect(root.model.safeRef).toBeUndefined();
133+
});
106134

107-
test("does not throw for invalid safe references", () => {
108-
const root = create(
109-
Root,
110-
{
111-
model: {
112-
ref: "item-a",
113-
safeRef: "item-c",
135+
test("references are equal to the instances they refer to", () => {
136+
const root = create(
137+
Root,
138+
{
139+
model: {
140+
ref: "item-a",
141+
safeRef: "item-b",
142+
},
143+
refs: [
144+
{ key: "item-a", count: 12 },
145+
{ key: "item-b", count: 523 },
146+
],
114147
},
115-
refs: [
116-
{ key: "item-a", count: 12 },
117-
{ key: "item-b", count: 523 },
118-
],
119-
},
120-
true,
121-
);
148+
readOnly,
149+
);
122150

123-
expect(root.model.safeRef).toBeUndefined();
124-
});
151+
expect(root.model.ref).toBe(root.refs[0]);
152+
expect(root.model.ref).toEqual(root.refs[0]);
153+
expect(root.model.ref).toStrictEqual(root.refs[0]);
154+
});
125155

126-
test("references are equal to the instances they refer to", () => {
127-
const root = create(
128-
Root,
129-
{
130-
model: {
131-
ref: "item-a",
132-
safeRef: "item-b",
156+
test("safe references are equal to the instances they refer to", () => {
157+
const root = create(
158+
Root,
159+
{
160+
model: {
161+
ref: "item-a",
162+
safeRef: "item-b",
163+
},
164+
refs: [
165+
{ key: "item-a", count: 12 },
166+
{ key: "item-b", count: 523 },
167+
],
133168
},
134-
refs: [
135-
{ key: "item-a", count: 12 },
136-
{ key: "item-b", count: 523 },
137-
],
138-
},
139-
true,
140-
);
169+
readOnly,
170+
);
141171

142-
expect(root.model.ref).toBe(root.refs[0]);
143-
expect(root.model.ref).toEqual(root.refs[0]);
144-
expect(root.model.ref).toStrictEqual(root.refs[0]);
145-
});
172+
expect(root.model.safeRef).toBe(root.refs[1]);
173+
expect(root.model.safeRef).toEqual(root.refs[1]);
174+
expect(root.model.safeRef).toStrictEqual(root.refs[1]);
175+
});
146176

147-
test("safe references are equal to the instances they refer to", () => {
148-
const root = create(
149-
Root,
150-
{
151-
model: {
152-
ref: "item-a",
153-
safeRef: "item-b",
177+
@register
178+
class Weird extends ClassModel({
179+
examples: types.map(NameExample),
180+
obj: types.union(types.reference(NameExample), Apple),
181+
}) {}
182+
183+
test("unions where one type is a reference can be resolved as references", () => {
184+
const instance = create(
185+
Weird,
186+
{
187+
examples: {
188+
foo: { key: "foo", name: "foo" },
189+
},
190+
obj: "foo",
154191
},
155-
refs: [
156-
{ key: "item-a", count: 12 },
157-
{ key: "item-b", count: 523 },
158-
],
159-
},
160-
true,
161-
);
192+
readOnly,
193+
);
194+
195+
expect((instance.obj as NameExample).name).toEqual("foo");
196+
});
197+
198+
test("unions where one type is a reference can be resolved a the non-reference type", () => {
199+
const instance = create(
200+
Weird,
201+
{
202+
examples: {
203+
foo: { key: "foo", name: "foo" },
204+
},
205+
obj: { type: "Apple", ripeness: 1 },
206+
},
207+
readOnly,
208+
);
162209

163-
expect(root.model.safeRef).toBe(root.refs[1]);
164-
expect(root.model.safeRef).toEqual(root.refs[1]);
165-
expect(root.model.safeRef).toStrictEqual(root.refs[1]);
210+
expect((instance.obj as Apple).ripeness).toEqual(1);
211+
});
166212
});
167213
});
168214

0 commit comments

Comments
 (0)