Skip to content

Commit

Permalink
[BUGFIX] Ensure owner is available during resolution for all components
Browse files Browse the repository at this point in the history
Previously, strict components were not passed an owner when they were
defined. This turned out to be incorrect, because even if strict
components did not use an owner to resolve components, the components
that they used in scope could potentially be non-strict, and resolve
components. Those components effectively become different components
based on the currently active owner, so we have to pass the owner no
matter what type of component we are rendering.
  • Loading branch information
Chris Garrett committed Feb 10, 2021
1 parent 2359354 commit 5d1e9c3
Show file tree
Hide file tree
Showing 6 changed files with 188 additions and 24 deletions.
157 changes: 156 additions & 1 deletion packages/@glimmer/integration-tests/test/owner-test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import { ResolvedComponentDefinition } from '@glimmer/interfaces';
import {
InternalComponentCapabilities,
Owner,
ResolvedComponentDefinition,
WithCreateInstance,
WithSubOwner,
} from '@glimmer/interfaces';
import {
test,
suite,
Expand All @@ -7,7 +13,11 @@ import {
JitRenderDelegate,
createTemplate,
TestJitRuntimeResolver,
GlimmerishComponent,
defineComponent,
} from '..';
import { NULL_REFERENCE, Reference } from '@glimmer/reference';
import { setInternalComponentManager } from '@glimmer/manager';

class OwnerJitRuntimeResolver extends TestJitRuntimeResolver {
lookupComponent(name: string, owner: () => void): ResolvedComponentDefinition | null {
Expand All @@ -21,6 +31,84 @@ class OwnerJitRenderDelegate extends JitRenderDelegate {
protected resolver = new OwnerJitRuntimeResolver(this.registry);
}

const CAPABILITIES = {
dynamicLayout: false,
dynamicTag: false,
prepareArgs: false,
createArgs: false,
attributeHook: false,
elementHook: false,
createCaller: false,
dynamicScope: false,
updateHook: false,
createInstance: true,
wrapped: false,
willDestroy: false,
hasSubOwner: true,
};

class MountComponent {
static owner: object;
}

class MountManager implements WithCreateInstance<object>, WithSubOwner<object> {
getCapabilities(): InternalComponentCapabilities {
return CAPABILITIES;
}

getOwner(state: object) {
return state;
}

create(_owner: Owner, state: typeof MountComponent) {
return state.owner;
}

getSelf(): Reference {
return NULL_REFERENCE;
}

didCreate() {}
didUpdate() {}

didRenderLayout() {}
didUpdateLayout() {}

getDestroyable() {
return null;
}

getDebugName() {
return 'mount';
}
}

setInternalComponentManager(new MountManager(), MountComponent);

function defineMountComponent(owner: object, scope: Record<string, unknown>, template: string) {
return defineComponent(
scope,
template,
class extends MountComponent {
static owner = owner;
}
);
}

function defineCheckOwnerComponent(ownerToCheck: object | undefined, assert: Assert) {
return defineComponent(
{},
'{{yield}}',
class extends GlimmerishComponent {
constructor(owner: object, args: Record<string, unknown>) {
super(owner, args);

assert.equal(owner, ownerToCheck, 'owner is correct');
}
}
);
}

class OwnerTest extends RenderTest {
static suiteName = '[owner]';

Expand Down Expand Up @@ -81,6 +169,73 @@ class OwnerTest extends RenderTest {

assert.verifySteps(['foo-bar owner called', 'foo-baz owner called'], 'owners used correctly');
}

@test
'owner can be changed by a component with the hasSubOwner capability'(assert: Assert) {
let owner1 = { name: 'owner1' };
let owner2 = { name: 'owner2' };

const CheckOwner2 = defineCheckOwnerComponent(owner1, assert);
const CheckOwner1 = defineCheckOwnerComponent(owner1, assert);

const Mount2 = defineMountComponent(owner2, { CheckOwner2 }, `<CheckOwner2/>{{yield}}`);
const Mount1 = defineMountComponent(
owner1,
{ CheckOwner1, Mount2 },
`<CheckOwner1/><Mount2><CheckOwner1/></Mount2>`
);

this.renderComponent(Mount1);
}

@test
'owner is preserved in curried closure components'(assert: Assert) {
let owner1 = { name: 'owner1' };
let owner2 = { name: 'owner2' };

const CheckOwner2 = defineCheckOwnerComponent(owner1, assert);
const CheckOwner1 = defineCheckOwnerComponent(owner1, assert);

const Mount2 = defineMountComponent(owner2, { CheckOwner2 }, `<CheckOwner2/><@CheckOwner1/>`);
const Mount1 = defineMountComponent(
owner1,
{ CheckOwner1, Mount2 },
`<CheckOwner1/><Mount2 @CheckOwner1={{component CheckOwner1}}/>`
);

this.renderComponent(Mount1);
}

// TODO: This behavior could be confusing the users, but currently we don't know of a way
// to ensure we are using the component with the correct owner if it was not curried.
// We should continue exploring options here.
@test
'owner is preserved in non-curried component definitions that are passed around'(assert: Assert) {
let owner1 = { name: 'owner1' };
let owner2 = { name: 'owner2' };

const CheckOwner2 = defineCheckOwnerComponent(owner1, assert);
const CheckOwner1 = defineCheckOwnerComponent(owner1, assert);

const Mount2 = defineMountComponent(owner2, { CheckOwner2 }, `<CheckOwner2/><@CheckOwner2/>`);
const Mount1 = defineMountComponent(
owner1,
{ CheckOwner1, CheckOwner2, Mount2 },
`<CheckOwner1/><Mount2 @CheckOwner2={{CheckOwner2}}/>`
);

this.renderComponent(Mount1);
}

@test
'resolution mode components defined within strict mode components receive correct owner during compilation'() {
this.registerComponent('TemplateOnly', 'Foo', 'Hello, world!');

const Bar = defineComponent(null, '<Foo/>');
const Baz = defineComponent({ Bar }, '<Bar/>');

this.renderComponent(Baz);
}
}

suite(OwnerTest, OwnerJitRenderDelegate);
12 changes: 6 additions & 6 deletions packages/@glimmer/interfaces/lib/program.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,16 +109,16 @@ export interface ResolutionTimeConstants {
): number | null;
modifier(definitionState: ModifierDefinitionState, resolvedName?: string | null): number;

component(definitionState: ComponentDefinitionState): ComponentDefinition;
component(
definitionState: ComponentDefinitionState,
isOptional: true
): ComponentDefinition | null;
owner: object,
isOptional?: false
): ComponentDefinition;
component(
definitionState: ComponentDefinitionState,
isOptional: false,
owner: Owner
): ComponentDefinition;
owner: object,
isOptional?: boolean
): ComponentDefinition | null;

resolvedComponent(
definitionState: ResolvedComponentDefinition,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,17 @@ export function resolveComponent(
}

if (type === SexpOpcodes.GetTemplateSymbol) {
let { scopeValues } = meta;
let { scopeValues, owner } = meta;
let definition = expect(scopeValues, 'BUG: scopeValues must exist if template symbol is used')[
expr[1]
];

then(constants.component(definition as object));
then(
constants.component(
definition as object,
expect(owner, 'BUG: expected owner when resolving component definition')
)
);
} else {
let { upvars, owner } = assertResolverInvariants(meta);

Expand Down Expand Up @@ -230,12 +235,16 @@ export function resolveComponentOrHelper(
let type = expr[0];

if (type === SexpOpcodes.GetTemplateSymbol) {
let { scopeValues } = meta;
let { scopeValues, owner } = meta;
let definition = expect(scopeValues, 'BUG: scopeValues must exist if template symbol is used')[
expr[1]
];

let component = constants.component(definition as object, true);
let component = constants.component(
definition as object,
expect(owner, 'BUG: expected owner when resolving component definition'),
true
);

if (component !== null) {
ifComponent(component);
Expand Down Expand Up @@ -324,7 +333,7 @@ export function resolveOptionalComponentOrHelper(
let type = expr[0];

if (type === SexpOpcodes.GetTemplateSymbol) {
let { scopeValues } = meta;
let { scopeValues, owner } = meta;
let definition = expect(scopeValues, 'BUG: scopeValues must exist if template symbol is used')[
expr[1]
];
Expand All @@ -338,7 +347,11 @@ export function resolveOptionalComponentOrHelper(
return;
}

let component = constants.component(definition, true);
let component = constants.component(
definition,
expect(owner, 'BUG: expected owner when resolving component definition'),
true
);

if (component !== null) {
ifComponent(component);
Expand Down
10 changes: 3 additions & 7 deletions packages/@glimmer/program/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,15 +196,11 @@ export class ConstantsImpl
return handle;
}

component(definitionState: ComponentDefinitionState): ComponentDefinition;
component(definitionState: ComponentDefinitionState, owner: object): ComponentDefinition;
component(
definitionState: ComponentDefinitionState,
isOptional: true
): ComponentDefinition | null;
component(
definitionState: ComponentDefinitionState,
isOptional?: true,
owner?: object
owner: object,
isOptional?: true
): ComponentDefinition | null {
let definition = this.componentDefinitionCache.get(definitionState);

Expand Down
6 changes: 3 additions & 3 deletions packages/@glimmer/runtime/lib/compiled/opcodes/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ APPEND_OPCODES.add(Op.ResolveDynamicComponent, (vm, { op1: _isStrict }) => {
} else if (isCurriedValue(component)) {
definition = component;
} else {
definition = constants.component(component);
definition = constants.component(component, owner);
}

stack.pushJs(definition);
Expand All @@ -178,7 +178,7 @@ APPEND_OPCODES.add(Op.ResolveCurriedComponent, (vm) => {
if (isCurriedValue(value)) {
definition = value;
} else {
definition = constants.component(value as object, true);
definition = constants.component(value as object, vm.getOwner(), true);

if (DEBUG && definition === null) {
throw new Error(
Expand Down Expand Up @@ -266,7 +266,7 @@ APPEND_OPCODES.add(Op.PrepareArgs, (vm, { op1: _state }) => {
value
);
} else {
definition = constants.component(value);
definition = constants.component(value, owner);
}

let { manager } = definition;
Expand Down
2 changes: 1 addition & 1 deletion packages/@glimmer/runtime/lib/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ function renderInvocation(
// Prefix argument names with `@` symbol
const argNames = argList.map(([name]) => `@${name}`);

let reified = vm[CONSTANTS].component(definition, false, owner);
let reified = vm[CONSTANTS].component(definition, owner);

vm.pushFrame();

Expand Down

0 comments on commit 5d1e9c3

Please sign in to comment.