Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Ensure owner is available during resolution for all components #1263

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Feb 10, 2021

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.

@pzuraq pzuraq force-pushed the pass-owner-to-all-component-definitions branch from 09a2729 to 609c3cd Compare February 10, 2021 01:24
@@ -81,6 +169,34 @@ 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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is unrelated, I just realized we didn't have it (or at least not an obvious one for it) so wanted to add it

Copy link
Contributor

@krisselden krisselden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test that passes a closure component into the model property of the mount and invoke that component inside of the mount with a yield block that invokes a component coming from the mount?

{{mount 'engine' model=MyComponent}}

inside the mounted

{{#let @model as |OuterComponent|}}
   <OuterComponent>
      <InnerComponent/>
  </OuterComponent>
{{/let}}

I want to ensure you didn't conflate the owner (realm) concept with dynamic scope.

@pzuraq pzuraq force-pushed the pass-owner-to-all-component-definitions branch 2 times, most recently from bc21723 to a90521e Compare February 10, 2021 21:10
@pzuraq pzuraq force-pushed the pass-owner-to-all-component-definitions branch from a90521e to 434ea5a Compare February 10, 2021 23:41
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.
@pzuraq pzuraq force-pushed the pass-owner-to-all-component-definitions branch from 434ea5a to 5d1e9c3 Compare February 10, 2021 23:54
Comment on lines +209 to +228
// 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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, we (@pzuraq + @krisselden + myself) discussed this in detail. The rough conclusion is that we don't really like the thing this test is asserting (it is very strange to end up with an owner from a different invocation than where you were originally associated), but it is quite difficult to ensure that we always have the right owner (it isn't even possible to absorb all variants of things that could be done in JavaScript land).

At the moment, we intend to land this update over in Ember and add a debug only AST transform that wraps any values passed to {{mount 'foo' model= (because as far as we can tell, that is the only way to pass things "across owner / realm") and asserts if you pass something that is a component/helper/modifier that isn't wrapped in (component/(helper/(modifier helpers.

@pzuraq pzuraq merged commit 5482bdd into master Feb 11, 2021
@pzuraq pzuraq deleted the pass-owner-to-all-component-definitions branch February 11, 2021 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants