Skip to content

Commit aa1b0e7

Browse files
NullVoxPopuliNullVoxPopuli
authored
Merge pull request #21203 from johanrd/test/20959
[BUGFIX] `@model` becomes `undefined` or changes to the wrong route's model during Glimmer component willDestroy
2 parents 605562c + c1129f2 commit aa1b0e7

2 files changed

Lines changed: 179 additions & 2 deletions

File tree

‎packages/@ember/-internals/glimmer/lib/syntax/outlet.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,15 +157,29 @@ export const outletHelper = /*@__PURE__*/ internalHelper(
157157
// Store the value of the model
158158
let model = valueForRef(modelRef);
159159

160+
// The controller for this outlet, used to verify the outletRef
161+
// still points to the correct route's data.
162+
let outletController = state.controller;
163+
160164
// Create a compute ref which we pass in as the `{{@model}}` reference
161165
// for the outlet. This ref will update and return the value of the
162166
// model _until_ the outlet itself changes. Once the outlet changes,
163167
// dynamic scope also changes, and so the original model ref would not
164168
// provide the correct updated value. So we stop updating and return
165169
// the _last_ model value for that outlet.
170+
//
171+
// We also verify that the outletRef still resolves to this route's
172+
// data by comparing controller identity. This handles the case where
173+
// a parent outlet is torn down first: the dynamic scope refs now
174+
// point to the new route's outlet state, but this outlet's outer
175+
// compute ref hasn't re-evaluated yet, so `lastState === state` is
176+
// still true. The controller check catches this case.
166177
named['model'] = createComputeRef(() => {
167178
if (lastState === state) {
168-
model = valueForRef(modelRef);
179+
let currentOutlet = valueForRef(outletRef);
180+
if (currentOutlet?.render?.controller === outletController) {
181+
model = valueForRef(modelRef);
182+
}
169183
}
170184

171185
return model;

‎smoke-tests/scenarios/basic-test.ts‎

Lines changed: 164 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,39 @@ function basicTest(scenarios: Scenarios, appName: string) {
1818
}
1919
2020
Router.map(function () {
21-
this.route('example-gjs-route')
21+
this.route('example-gjs-route');
22+
this.route('a', function () {
23+
this.route('b');
24+
this.route('c');
25+
});
26+
this.route('d', function () {
27+
this.route('e');
28+
});
29+
this.route('f');
30+
this.route('item', { path: '/item/:item_id' });
31+
this.route('g', function () {
32+
this.route('h', function () {
33+
this.route('i');
34+
});
35+
});
2236
});
2337
`,
2438
components: {
39+
'model-probe.gjs': `
40+
import Component from '@glimmer/component';
41+
42+
const destroyedModels = [];
43+
export function getDestroyedModels() { return destroyedModels; }
44+
export function clearDestroyedModels() { destroyedModels.length = 0; }
45+
46+
export default class ModelProbe extends Component {
47+
willDestroy() {
48+
super.willDestroy();
49+
destroyedModels.push(this.args.model);
50+
}
51+
<template>{{@model}}</template>
52+
}
53+
`,
2554
'interactive-example.js': `
2655
import { template } from '@ember/template-compiler';
2756
import Component from '@glimmer/component';
@@ -66,6 +95,54 @@ function basicTest(scenarios: Scenarios, appName: string) {
6695
}
6796
}
6897
`,
98+
'a.js': `
99+
import Route from '@ember/routing/route';
100+
export default class extends Route { model() { return 'a'; } }
101+
`,
102+
a: {
103+
'b.js': `
104+
import Route from '@ember/routing/route';
105+
export default class extends Route { model() { return 'b'; } }
106+
`,
107+
'c.js': `
108+
import Route from '@ember/routing/route';
109+
export default class extends Route { model() { return 'c'; } }
110+
`,
111+
},
112+
'd.js': `
113+
import Route from '@ember/routing/route';
114+
export default class extends Route { model() { return 'd'; } }
115+
`,
116+
d: {
117+
'e.js': `
118+
import Route from '@ember/routing/route';
119+
export default class extends Route { model() { return 'e'; } }
120+
`,
121+
},
122+
'f.js': `
123+
import Route from '@ember/routing/route';
124+
export default class extends Route { model() { return 'f'; } }
125+
`,
126+
'item.js': `
127+
import Route from '@ember/routing/route';
128+
export default class extends Route { model(params) { return params.item_id; } }
129+
`,
130+
'g.js': `
131+
import Route from '@ember/routing/route';
132+
export default class extends Route { model() { return 'g'; } }
133+
`,
134+
g: {
135+
'h.js': `
136+
import Route from '@ember/routing/route';
137+
export default class extends Route { model() { return 'h'; } }
138+
`,
139+
h: {
140+
'i.js': `
141+
import Route from '@ember/routing/route';
142+
export default class extends Route { model() { return 'i'; } }
143+
`,
144+
},
145+
},
69146
},
70147
templates: {
71148
'example-gjs-route.gjs': `
@@ -83,6 +160,27 @@ function basicTest(scenarios: Scenarios, appName: string) {
83160
</template>
84161
}
85162
`,
163+
'a.gjs': `<template>{{outlet}}</template>`,
164+
a: {
165+
'b.gjs': `
166+
import ModelProbe from '${appName}/components/model-probe';
167+
<template><ModelProbe @model={{@model}} /></template>
168+
`,
169+
},
170+
'item.gjs': `
171+
import ModelProbe from '${appName}/components/model-probe';
172+
<template><ModelProbe @model={{@model}} /></template>
173+
`,
174+
'g.gjs': `<template>{{outlet}}</template>`,
175+
g: {
176+
'h.gjs': `<template>{{outlet}}</template>`,
177+
h: {
178+
'i.gjs': `
179+
import ModelProbe from '${appName}/components/model-probe';
180+
<template><ModelProbe @model={{@model}} /></template>
181+
`,
182+
},
183+
},
86184
},
87185
},
88186
tests: {
@@ -118,6 +216,71 @@ function basicTest(scenarios: Scenarios, appName: string) {
118216
});
119217
});
120218
`,
219+
'model-stability-test.js': `
220+
import { module, test } from 'qunit';
221+
import { visit } from '@ember/test-helpers';
222+
import { setupApplicationTest } from '${appName}/tests/helpers';
223+
import { getDestroyedModels, clearDestroyedModels } from '${appName}/components/model-probe';
224+
225+
module('Acceptance | @model stability during route transitions', function (hooks) {
226+
setupApplicationTest(hooks);
227+
hooks.beforeEach(function () { clearDestroyedModels(); });
228+
229+
test('@model should be stable when transitioning out of the route', async function (assert) {
230+
await visit('/a/b');
231+
await visit('/a');
232+
233+
await visit('/a/b');
234+
await visit('/a/c');
235+
236+
await visit('/a/b');
237+
await visit('/d');
238+
239+
await visit('/a/b');
240+
await visit('/d/e');
241+
242+
await visit('/a/b');
243+
await visit('/f');
244+
245+
assert.deepEqual(
246+
getDestroyedModels(),
247+
['b', 'b', 'b', 'b', 'b'],
248+
'The @model value should remain stable in willDestroy for all transition types'
249+
);
250+
});
251+
252+
test('@model should update when the model changes on the same route', async function (assert) {
253+
await visit('/item/first');
254+
assert.dom().containsText('first');
255+
256+
await visit('/item/second');
257+
assert.dom().containsText('second');
258+
259+
await visit('/item/third');
260+
assert.dom().containsText('third');
261+
262+
// Leave the route entirely — the destroyed model should be the latest one
263+
await visit('/f');
264+
265+
assert.deepEqual(
266+
getDestroyedModels(),
267+
['third'],
268+
'The @model value should be the latest model when finally destroyed'
269+
);
270+
});
271+
272+
test('@model should be stable when grandparent outlet tears down', async function (assert) {
273+
await visit('/g/h/i');
274+
await visit('/f');
275+
276+
assert.deepEqual(
277+
getDestroyedModels(),
278+
['i'],
279+
'The @model value should remain stable when grandparent outlet tears down'
280+
);
281+
});
282+
});
283+
`,
121284
},
122285
integration: {
123286
'tracked-built-ins-macro-test.gjs': `

0 commit comments

Comments
 (0)