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

Composing a view can access the parent property #222

Open
leon-andria opened this issue May 11, 2016 · 16 comments
Open

Composing a view can access the parent property #222

leon-andria opened this issue May 11, 2016 · 16 comments

Comments

@leon-andria
Copy link

leon-andria commented May 11, 2016

We have 2 views: main.html and child.html.
Inside main.html, we do a compose on child.html:

 <compose view-model="child"></compose>

Note that no model is passed.
Now, in main.js, we specify in the activate method:

this.manouche = "Hello";

Now, in child.html:

  <p>${manouche}</p>

The result is that in the child.html, we got "Hello".
Does this is an expected behavior or it's a bug ? This means Compose can inherits from its parent ?

@alvarezmario
Copy link

@leon-andria yep, compose inherits from its parent. In any case, someone from the core team should be more suitable for the final answer

@alvarezmario
Copy link

in fact, according to this https://github.com/aurelia/templating-resources/blob/master/src/compose.js#L73 is in fact the expected behavior

@EisenbergEffect
Copy link
Contributor

Composing a view automatically inherits its binding context from the parent if no view-model is provided, otherwise there would be nothing to bind to. If this is not desired, you can provide a fake view model like this view-model.bind="{}". I haven't tried that myself, but it should work. Let me know if it doesn't.

@leon-andria
Copy link
Author

leon-andria commented May 11, 2016

@rob, thanks for your clarification, it's what I expected before to ask if it's a bug or not ?
So, with the following code:

<compose view-model.bind="child"
                 model.bind="model">

In my child, I still get "Hello" from the parent.
This means that Compose doesn't isolate anymore ? Is it a design choice ?
For my understanding from the documentation, when you do this in a "repeat.for", it makes sense but here it's not my case.

@EisenbergEffect
Copy link
Contributor

Yes, the compose element enables flowing through the binding context and override context. This is different from standard components which are fully encapsulated. This means that with compose, if the binding engine doesn't find a match on the composed view model, it can walk the binding context tree. (Traditional components can't because the trees aren't connected.)

We wanted to provide this as a possibility for certain scenarios where it made sense. However, it's not advisable to take advantage of this except in rare cases, since it creates unnecessary coupling from child to parent.

If you would like an option to explicitly prevent this, we could extend the compose element with a property "inherit-context" which could be used to turn this off. It would be very simple to add. In the link referenced above we would simply not pass the context of the compose element along to the composition engine. If we don't do that, it cannot connect them.

Let me know your thoughts on this. Also, I'd love to hear from @alvarezmario and @jdanyow

@leon-andria
Copy link
Author

@rob,
This behavior is very useful but we just need to be careful when we provide guidelines and conventions in our development process. Guess that you have "this.model" naming convention in each view, when you review the code, you will say that it's magic because were "model.xxx" is defined ?:-)

"inherit-context=true" would be good if you want it explicitly. Otherwise, it's like very odd that you put it off and inside the view it-self you know that you are not allow to use parent context.

For me, keep it like this but highlight this in the documentation of Compose.

However, I would expect to use a predefined property to explicitly get my parent context, rather than just ${monouche}, i would like ${ViewParentContext.manouche}. This avoid property scope confusion.

@geertendoornenbal
Copy link

I read this, and was very intrigued. We encountered exactly this issue, where by accident a binding 'happened' because of the larger scope with a compose.
For us, an "inherit-context=false" would be better, to prevent sloppiness.. The solution of @leon-andria is also interesting, and in my opinion the cleaner solution, so you always explicitly need to 'drag' it in..
@EisenbergEffect , if you say this is easy to implement, could you add this? That would be much appreciated :)
We are adopting Aurelia massively here, and starting multiple projects with it at the moment. So the cleaner it can get, the better it is!

@EisenbergEffect
Copy link
Contributor

If we were starting over, we would probably not inherit the context by default, but require the property to be set first. However, since we made the opposite default choice, and we don't want to break backwards compatibility at this point, the only solution I see is to add the ability to turn this off. Again, it's not perfect, but it should enable the scenario. In a future breaking change, we could flip-flop the default. We're not quite ready to do that just yet though. Does that make sense?

@geertendoornenbal
Copy link

I understand your reasoning for not wanting to break backwards compatibility. An option to turn it off is fine as a workaround!
Thanks!

@suamikim
Copy link

I'd also appreciate such an option!

@leon-andria
Copy link
Author

@EisenbergEffect To keep compatibility, having a global setting

compose-context-behavior = 'Cascading | Block'

The default is cascading to make sure that we keep the compatibility.
So, for any new Applications and aware of this, you can specify 'Block' and when you are in your compose, you can call from ${ViewParentContext.xxx}

Since we opened this issue, we got a lot of issue from many developper and changed some guidelines in our process. Think it like having 10 more developpers working in an Application and there is a scoping collision because we use compose feature, it was a nightmare to find out the bug.
It's like you want to say, don't use compose... This was the lesson we learnt!

@jdanyow
Copy link
Contributor

jdanyow commented Apr 18, 2017

What if we did this:

  1. view -only compose continues to inherit the binding context (no change).
    <compose view="something.html"></compose>`
    <compose view.bind="myProperty"></compose>`
  2. view-model compose does not inherit the binding context (change).
    <compose view-model="something"></compose>`
    <compose view-model.bind="myProperty"></compose>`
    <compose view-model="something" view="another.html"></compose>`
  3. Developers can force the compose to inherit the binding context by adding an inherit-binding-context attribute to the compose element (change).
    <compose view-model="something" inherit-binding-context></compose>`
  4. For a period of time (TBD), aurelia.use.legacyComposeBehavior() will preserve the compose behavior we have today (change).

I think for the most part this will do what developers expect and won't require any changes for typical compose use-cases.

@EisenbergEffect, all, thoughts?

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Apr 18, 2017 via email

@3cp
Copy link
Member

3cp commented Jan 13, 2018

@jdanyow, is it on the near future roadmap to implement your proposal?

I also want to suggest an amendment. I really want to use inherit-binding-context attribute not only on compose tag, but also on any custom component tag. So that we can turn it on like <child inherit-binding-context></child> without using verbose @processContent annotation.

I am one of the minority who want to use inheritBindingContext for some cases. Hope we are also covered by Aurelia. Thanks.

@szogun1987
Copy link

szogun1987 commented Feb 27, 2018

I use 1.1.5 version of framework. I found following issue related to this improvement:
I have view-model that contains router in it. It has "data" field. One of the view-models displayed in router-view also contains "data" field that is loaded asynchronously. Data from parent view-model is displayed until data is loaded. In my opinion value from parent shouldn't be used if local view-model contains expected field (even if it is undefined).
Edit
Issue doesn't occur if data is null

@3cp
Copy link
Member

3cp commented Feb 27, 2018

@szogun1987 as a workaround, you can predefine a placeholder for data field instead of waiting async creating, so it can over-shadow the data of parent binding context.

export class ChildVM {
  data = null;
  // async updates data field
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants