Skip to content

Commit 2a5515d

Browse files
NullVoxPopuliNullVoxPopuli
authored
Merge pull request #21224 from NullVoxPopuli-ai-agent/nvp/named-invocations
Make the low-level component scope able to be an object instead of an array
2 parents 6c66561 + 88bb0d1 commit 2a5515d

File tree

10 files changed

+166
-16
lines changed

10 files changed

+166
-16
lines changed

‎packages/@glimmer-workspace/integration-tests/lib/compile.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ export function createTemplate(
2424
): TemplateFactory {
2525
options.locals = options.locals ?? Object.keys(scopeValues ?? {});
2626
let [block, usedLocals] = precompileJSON(templateSource, options);
27-
let reifiedScopeValues = usedLocals.map((key) => scopeValues[key]);
27+
let reifiedScope: Record<string, unknown> = {};
28+
for (let key of usedLocals) {
29+
reifiedScope[key] = scopeValues[key];
30+
}
2831

2932
if ('emit' in options && options.emit?.debugSymbols) {
3033
block.push(usedLocals);
@@ -34,7 +37,7 @@ export function createTemplate(
3437
id: String(templateId++),
3538
block: JSON.stringify(block),
3639
moduleName: options.meta?.moduleName ?? '(unknown template module)',
37-
scope: reifiedScopeValues.length > 0 ? () => reifiedScopeValues : null,
40+
scope: usedLocals.length > 0 ? () => reifiedScope : null,
3841
isStrictMode: options.strictMode ?? false,
3942
};
4043

‎packages/@glimmer-workspace/integration-tests/test/compiler/compile-options-test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ module('[glimmer-compiler] precompile', ({ test }) => {
5757
...WireFormat.Statement[],
5858
];
5959

60-
assert.deepEqual(wire.scope?.(), [hello]);
60+
assert.deepEqual(wire.scope?.(), { hello });
6161

6262
assert.deepEqual(
6363
componentNameExpr,
@@ -84,7 +84,7 @@ module('[glimmer-compiler] precompile', ({ test }) => {
8484
...WireFormat.Statement[],
8585
];
8686

87-
assert.deepEqual(wire.scope?.(), [f]);
87+
assert.deepEqual(wire.scope?.(), { f });
8888
assert.deepEqual(
8989
componentNameExpr,
9090
[SexpOpcodes.GetLexicalSymbol, 0, ['hello']],
@@ -218,7 +218,7 @@ module('[glimmer-compiler] precompile', ({ test }) => {
218218
_wire = compile(`{{this.message}}`, ['this'], (source) => eval(source));
219219
}).call(target);
220220
let wire = _wire!;
221-
assert.deepEqual(wire.scope?.(), [target]);
221+
assert.deepEqual(wire.scope?.(), { this: target });
222222
assert.deepEqual(wire.block[0], [
223223
[SexpOpcodes.Append, [SexpOpcodes.GetLexicalSymbol, 0, ['message']]],
224224
]);

‎packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type {
1111
import type { TemplateOnlyComponent } from '@glimmer/runtime';
1212
import type { EmberishCurlyComponent } from '@glimmer-workspace/integration-tests';
1313
import { expect } from '@glimmer/debug-util';
14+
import { DEBUG } from '@glimmer/env';
1415
import { modifierCapabilities, setComponentTemplate, setModifierManager } from '@glimmer/manager';
1516
import { EMPTY_ARGS, templateOnlyComponent, TemplateOnlyComponentManager } from '@glimmer/runtime';
1617
import { assign } from '@glimmer/util';
@@ -110,6 +111,88 @@ class DebugRenderTreeTest extends RenderTest {
110111
]);
111112
}
112113

114+
@test 'strict-mode components without debug symbols preserve names from scope'() {
115+
const HelloWorld = defComponent('{{@arg}}');
116+
const Root = defComponent(`<HelloWorld @arg="first"/>`, {
117+
scope: { HelloWorld },
118+
emit: { moduleName: 'root.hbs', debugSymbols: false },
119+
});
120+
121+
this.renderComponent(Root);
122+
123+
this.assertRenderTree([
124+
{
125+
type: 'component',
126+
name: '{ROOT}',
127+
args: { positional: [], named: {} },
128+
instance: null,
129+
template: 'root.hbs',
130+
bounds: this.elementBounds(this.delegate.getInitialElement()),
131+
children: [
132+
{
133+
type: 'component',
134+
name: 'HelloWorld',
135+
args: { positional: [], named: { arg: 'first' } },
136+
instance: null,
137+
template: '(unknown template module)',
138+
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
139+
children: [],
140+
},
141+
],
142+
},
143+
]);
144+
}
145+
146+
@test({ skip: !DEBUG }) 'dynamic component via <this.dynamicComponent>'() {
147+
const HelloWorld = defComponent('{{@arg}}');
148+
149+
class Root extends GlimmerishComponent {
150+
HelloWorld = HelloWorld;
151+
}
152+
153+
const RootDef = defComponent(`<this.HelloWorld @arg="first"/>`, {
154+
component: Root,
155+
emit: { moduleName: 'root.hbs' },
156+
});
157+
158+
this.renderComponent(RootDef);
159+
160+
const rootChildren = this.delegate.getCapturedRenderTree()[0]?.children ?? [];
161+
const componentNode = rootChildren.find(
162+
(n: CapturedRenderNode) => n.type === 'component' && n.name !== '{ROOT}'
163+
);
164+
165+
this.assert.ok(componentNode, 'found a component child node');
166+
167+
this.assert.strictEqual(
168+
componentNode?.name,
169+
'this.HelloWorld',
170+
`dynamic <this.X> component name (got "${componentNode?.name}")`
171+
);
172+
}
173+
174+
@test({ skip: !DEBUG }) 'dynamic component via <@argComponent>'() {
175+
const HelloWorld = defComponent('{{@arg}}');
176+
const Root = defComponent(`<@Greeting @arg="first"/>`, {
177+
emit: { moduleName: 'root.hbs' },
178+
});
179+
180+
this.renderComponent(Root, { Greeting: HelloWorld });
181+
182+
const rootChildren = this.delegate.getCapturedRenderTree()[0]?.children ?? [];
183+
const componentNode = rootChildren.find(
184+
(n: CapturedRenderNode) => n.type === 'component' && n.name !== '{ROOT}'
185+
);
186+
187+
this.assert.ok(componentNode, 'found a component child node');
188+
189+
this.assert.strictEqual(
190+
componentNode?.name,
191+
'@Greeting',
192+
`dynamic <@X> component name (got "${componentNode?.name}")`
193+
);
194+
}
195+
113196
@test 'strict-mode modifiers'() {
114197
const state = trackedObj({ showSecond: false });
115198

‎packages/@glimmer/compiler/lib/compiler.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,14 @@ export function precompile(
148148
let stringified = JSON.stringify(templateJSONObject);
149149

150150
if (usedLocals.length > 0) {
151-
const scopeFn = `()=>[${usedLocals.join(',')}]`;
151+
const scopeEntries = usedLocals.map((name) => {
152+
// Reserved words like "this" can't use shorthand property syntax
153+
if (name === 'this') {
154+
return `"this":this`;
155+
}
156+
return name;
157+
});
158+
const scopeFn = `()=>({${scopeEntries.join(',')}})`;
152159

153160
stringified = stringified.replace(`"${SCOPE_PLACEHOLDER}"`, scopeFn);
154161
}

‎packages/@glimmer/interfaces/lib/compile/wire-format/api.d.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@ export interface SerializedTemplateWithLazyBlock {
397397
id?: Nullable<string>;
398398
block: SerializedTemplateBlockJSON;
399399
moduleName: string;
400-
scope?: (() => unknown[]) | undefined | null;
400+
scope?: (() => Record<string, unknown>) | undefined | null;
401401
isStrictMode: boolean;
402402
}
403403

‎packages/@glimmer/interfaces/lib/template.d.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ export interface LayoutWithContext {
1818
readonly block: SerializedTemplateBlock;
1919
readonly moduleName: string;
2020
readonly owner: Owner | null;
21-
readonly scope: (() => unknown[]) | undefined | null;
21+
readonly scope: (() => Record<string, unknown>) | undefined | null;
2222
readonly isStrictMode: boolean;
2323
}
2424

‎packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/shared.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,14 +107,15 @@ export function CompilePositional(
107107

108108
export function meta(layout: LayoutWithContext): BlockMetadata {
109109
let [, locals, upvars, lexicalSymbols] = layout.block;
110+
let scopeRecord = layout.scope?.() ?? null;
110111

111112
return {
112113
symbols: {
113114
locals,
114115
upvars,
115-
lexical: lexicalSymbols,
116+
lexical: scopeRecord ? Object.keys(scopeRecord) : lexicalSymbols,
116117
},
117-
scopeValues: layout.scope?.() ?? null,
118+
scopeValues: scopeRecord ? Object.values(scopeRecord) : null,
118119
isStrictMode: layout.isStrictMode,
119120
moduleName: layout.moduleName,
120121
owner: layout.owner,

‎packages/@glimmer/runtime/lib/compiled/opcodes/component.ts‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,10 +154,8 @@ APPEND_OPCODES.add(VM_PUSH_COMPONENT_DEFINITION_OP, (vm, { op1: handle }) => {
154154

155155
APPEND_OPCODES.add(VM_RESOLVE_DYNAMIC_COMPONENT_OP, (vm, { op1: _isStrict }) => {
156156
let stack = vm.stack;
157-
let component = check(
158-
valueForRef(check(stack.pop(), CheckReference)),
159-
CheckOr(CheckString, CheckCurriedComponentDefinition)
160-
);
157+
let ref = check(stack.pop(), CheckReference);
158+
let component = check(valueForRef(ref), CheckOr(CheckString, CheckCurriedComponentDefinition));
161159
let constants = vm.constants;
162160
let owner = vm.getOwner();
163161
let isStrict = constants.getValue<boolean>(_isStrict);
@@ -182,6 +180,13 @@ APPEND_OPCODES.add(VM_RESOLVE_DYNAMIC_COMPONENT_OP, (vm, { op1: _isStrict }) =>
182180
definition = constants.component(component, owner);
183181
}
184182

183+
if (DEBUG && !isCurriedValue(definition) && !definition.resolvedName && !definition.debugName) {
184+
let debugLabel = ref.debugLabel;
185+
if (debugLabel) {
186+
definition.debugName = debugLabel;
187+
}
188+
}
189+
185190
stack.push(definition);
186191
});
187192

@@ -217,6 +222,19 @@ APPEND_OPCODES.add(VM_RESOLVE_CURRIED_COMPONENT_OP, (vm) => {
217222
}
218223
}
219224

225+
if (
226+
DEBUG &&
227+
definition &&
228+
!isCurriedValue(definition) &&
229+
!definition.resolvedName &&
230+
!definition.debugName
231+
) {
232+
let debugLabel = ref.debugLabel;
233+
if (debugLabel) {
234+
definition.debugName = debugLabel;
235+
}
236+
}
237+
220238
stack.push(definition);
221239
});
222240

‎packages/internal-test-helpers/lib/compile.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,15 @@ export default function compile(
2424
): TemplateFactory {
2525
options.locals = options.locals ?? Object.keys(scopeValues ?? {});
2626
let [block, usedLocals] = precompileJSON(templateSource, compileOptions(options));
27-
let reifiedScopeValues = usedLocals.map((key) => scopeValues[key]);
27+
let reifiedScope: Record<string, unknown> = {};
28+
for (let key of usedLocals) {
29+
reifiedScope[key] = scopeValues[key];
30+
}
2831

2932
let templateBlock: SerializedTemplateWithLazyBlock = {
3033
block: JSON.stringify(block),
3134
moduleName: options.moduleName ?? options.meta?.moduleName ?? '(unknown template module)',
32-
scope: reifiedScopeValues.length > 0 ? () => reifiedScopeValues : null,
35+
scope: usedLocals.length > 0 ? () => reifiedScope : null,
3336
isStrictMode: options.strictMode ?? false,
3437
};
3538

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,41 @@ function basicTest(scenarios: Scenarios, appName: string) {
223223
224224
});
225225
`,
226+
'debug-render-tree-test.gjs': `
227+
import { module, test } from 'qunit';
228+
import { setupRenderingTest } from 'ember-qunit';
229+
import { render } from '@ember/test-helpers';
230+
import { captureRenderTree } from '@ember/debug';
231+
import Component from '@glimmer/component';
232+
233+
function flattenTree(nodes) {
234+
let result = [];
235+
for (let node of nodes) {
236+
result.push(node);
237+
if (node.children) {
238+
result.push(...flattenTree(node.children));
239+
}
240+
}
241+
return result;
242+
}
243+
244+
class HelloWorld extends Component {
245+
<template>{{@arg}}</template>
246+
}
247+
248+
module('Integration | captureRenderTree', function (hooks) {
249+
setupRenderingTest(hooks);
250+
251+
test('scope-based components have correct names in debugRenderTree', async function (assert) {
252+
await render(<template><HelloWorld @arg="first" /></template>);
253+
254+
let tree = captureRenderTree(this.owner);
255+
let allNodes = flattenTree(tree);
256+
let names = allNodes.filter(n => n.type === 'component').map(n => n.name);
257+
assert.true(names.includes('HelloWorld'), 'HelloWorld component name is preserved in the render tree (found: ' + names.join(', ') + ')');
258+
});
259+
});
260+
`,
226261
},
227262
},
228263
});

0 commit comments

Comments
 (0)