aurelia / templating-resources

A standard set of behaviors, converters and other resources for use with the Aurelia templating library.
MIT License
59 stars 55 forks source link

Composing a view can access the parent property #222

Open leon-andria opened 8 years ago

leon-andria commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

EisenbergEffect commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

suamikim commented 7 years ago

I'd also appreciate such an option!

leon-andria commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

Yes. Let's do it 😀

On Apr 18, 2017 7:17 AM, "Jeremy Danyow" notifications@github.com wrote:

What if we did this:

  1. view -only compose continues to inherit the binding context (no change).

    ` `
  2. view-model compose does not inherit the binding context (change).

    ` ` `
  3. Developers can force the compose to inherit the binding context by adding an inherit-binding-context attribute to the compose element (change).

    `
  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 https://github.com/EisenbergEffect, all, thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aurelia/templating-resources/issues/222#issuecomment-294858777, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIBncZlDKSlwgHAsT-r59Jz4PV7bznwks5rxMXfgaJpZM4IcQh1 .

3cp commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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
}