Consider adding an option to control AccessMember's automatic object creation

[jdanyow]

AccessMember automatically creates an object when assigning a value to a property on an object that doesn't exist.

Some people love this behavior. Other people don't like it.

Maybe we can make the behavior configurable?

cc @EisenbergEffect, @danfma

[EisenbergEffect]

Hmm. Yeah...I remember when I added that...a long time ago. We should definitely make this configurable. It's great for demos...but not sure it it's great for real world development. We should leave the default setting the way it is today, but enable to be swapped. Not sure what is the best way to handle settings like this. Recommendations welcome. @jdanyow You are the binding expert :)

[danfma]

To be honest, I really like this behavior except when it is used with the if attribute binding, because if I remove the inner nodes, I don't want the removed bindings of these dead nodes to continue to recreate things that are not there anymore.

But for the rest of the application is very fine!!

[danfma]

( Don't get me wrong!! I love this framework! hehe 👍 )

[jdanyow]

In terms of the alternate behavior, would you both agree that instead of creating an object the assignment should just fail silently, no errors thrown?

[danfma]

@jdanyow, did you understand that I'm talking about only the if binding? If yes, in my opinion, all inner bindings should be inactive or removed, so it will not affected the default behaviour and will work like knockout, for example.

[devmondo]

@jdanyow in our application, we really need this, and it should be deep and not shallow binding, because we have very complex and dynamic nature of data and the way it should be bound and sometimes user needs to create it.

thanks for taking care of it.

[leon-andria]

@rob, this feature is very important in real world application.
Let me give some valid argument that we do a million times every days since that we work with aurelia since few months.
We are building a first class web content edition with aurelia which allows content authors to edit content.
There are many ways to achieve that but the outcome is repeated code and not maintainable.

1. The simple quick and dirty is to have a custom editor for each page layout ou component by naminf convention that activate the editor dynamically
2. Defining a fake object as a dummy data as a preview
3. Having a manifest like a schema to define your Poco
4. We can generate the editor based by 3)

All of these can be an acceprable solution but how much effort to define such contract for any binding when you want to edit a content and persist it back to your server when you have elasticsearch or mangodb playing with....

If i may as we love convention and until this rule make the simplicity of aurelia, why not to detect array members when its name ended by ...Collection or ...Items?
When you ask a front developper to bind data, he will never think about manifest nor a poco.
Based on its binding, we help him to build his tree model and can be persisted naturally in json.

Leon

[jdanyow]

We will not remove the feature- we're only talking about adding an option to disable it.

[leon-andria]

@jdanyow, thank you for your support.
There is a chance that by naming convention, an array or map can be initialized.
...Items or ...Collection, ...KeyValues
This feature is very usefull and avoid to pre instantiate the property in an object hierarchy.
Tks

Leon

[devmondo]

@jdanyow also i must mention that this feature currently does not do deep and recursive binding, it is only one level deep, so it works on this model.name but not model.componentes.savedComponents so could you please look at that.

[jdanyow]

@leon-andria / @devmondo I don't think there's any plan to add further support for this "feature" for the beta. The intent of this issue is to add an option to turn the existing behavior off for those that want more explicit/predictable behavior.

[akircher]

I agree with @danfma, the only problem here is the combination of this feature with if.bind, show.bind, and ternary ? : operations. I like the feature in general, but it creates odd behavior with if and show.

An option to turn off existing behavior, may work but I don't think it would be very intuitive. For my issue aurelia/templating#83, I don't think it would occur to me or a common user to "turn off the automatic object creation feature". As a result unless the option's syntax is very clear and intuitive, you will have to keep on closing github issues and pointing people to use this option.

Alternatively, is there a way these automatic objects can evaluate to false when coerced to a boolean? I believe that would solve the above issues.

[EisenbergEffect]

That's good feedback. @jdanyow What do you think?

[jdanyow]

The problem manifests with if.bind but it's not the if binding command that's causing the problem, it's the input value binding to a path expression whose object is null.

I agree with @akircher's concern that people will have a difficult time figuring out there's an option they need to turn on to avoid this behavior. I would love to make the default behavior be "don't automatically create objects" and have an option that turns on that behavior. I think that would be most intuitive- people would need to opt-in to the magic.

Taking things a step further, it might be better to turn of the behavior entirely and don't add an option for it (because it will cause problems in situations where a plugin expects the behavior to be turned off and the app has turned it on). Instead maybe we could add a binding behavior that people can use in specific locations where they want automatic object creation.

This is part of the reason it's taken so long to get this issue resolved- I've been a little conflicted as to how it should be done. The binding behavior approach didn't occur to me until just now.

Thoughts?

[EisenbergEffect]

I like the idea of the binding behavior to opt in. Can you list out here some sample code for the different scenarios where we would and would not want the automatic creation? Seeing those altogether would be helpful. If you can create the psuedo code in terms of your proposed binding handler, that would be optimal :) I'd just like to get a feel for it from the developer's perspective.

[akircher]

In ecma script discussions, there was a thought of adding an "existential operator" such as deeply?.nested?.field which would allow the user to opt into the behavior and be eventually/possibly consistent with javascript syntax.

[EisenbergEffect]

That's in interesting idea. Did they do something like that for C# recently. @jdanyow I think this may be something worth considering.

[leon-andria]

Bases with our expériences, having the option off/on is not really helpful for the développer because it requîtes so much dependencies between plugins on your on apps. This is really dangerous for application design and sustainabilities. Today having it off is painfull because the if.bind syntaxe polluate your html code for visibility but this choice is douable to make it off. I would prefer on but this means that the binding engins takes more responsabilities and may ne confusing for object type création.
A trade off option is really using a design pattern of délégation or contexte.
Having someone to validate a model with schéma would be very usefulll and help the engine to create of the object instance or to delegate the création.

[jdanyow]

I think the "optional chaining" / "null propagation" spec only covers accessing properties. Aurelia's binding system already matches that behavior when accessing props on objects that don't exist.

The issue here has to do with property assignment- a few developers are defining their object structure in their HTML templates instead of creating the object structure ahead of time and binding to it. To be clear, we're not talking about binding to undefined properties here, we're talking about binding to properties of objects that are not yet defined or null- eg value.bind="undefinedObject.someProperty". Aurelia's behavior is not consistent, for example, value.bind="undefinedObject.someProperty" does not work the same as value.bind="undefinedObject['someProperty']". The former has magic object creation and the latter does not.

I haven't used the automatic object creation in any apps but I have had to work-around it... in other words I'm probably not the best advocate for this feature 😜 Definitely need some feedback here!

Here's some pseudo code for the binding behavior approach:

Without automatically creating objects:

<input value.bind="foo.bar.baz">

Opting in to automatically creating objects:

<input value.bind="foo.bar.baz & auto">

Opting in to automatically creating objects and providing an object factory:

<input value.bind="foo.lorem.ipsum & auto:factory"> <!-- factory is a function that accepts an AccessMember expression and returns an object -->

I'm not too sure about the name of the behavior... auto is pretty ambiguous. Also not sure about the optional factory argument...

[akircher]

@jdanyow Yes, you are right the spec doesn't handle this specific need. My thought is just that I think a new type of syntax here might be more intuitive than trying to solve this issue with binding behaviors.

<input value.bind="foo.bar.baz">
Throws error if you try to get/set baz when foo or bar are null.

<input value.bind="foo?.bar?.baz">
Enables null property access and object creation when foo or bar are null.

Edit: You also get an extra degree of specificity, since you could do foo.bar?.baz which would not be possible with binding behaviors (although not sure if that is actually solving a need or not).

[leon-andria]

@jdanyow: I like the idea, for a big app, it will be hard to governe rules for this.
By keeping your idea, rather having auto:factory, it will be good to have a chaining pattern handler

[jdanyow]

@akircher I like the specificity your proposal enables but throwing when accessing foo.bar.baz when foo or bar is null or undefined would be a big change in behavior.

A lot of people are getting tripped up by the automatic object creation, there are currently 8 issues that have been closed as duplicates of this issue. I was hoping we could turn it off while providing a path forward for those that want the binding system to instantiate objects. The binding behavior approach would accomplish this. Not as flexible as your approach but less disruptive. I was thinking the number of situations where someone wants/expects the automatic object creation is minimal, so there's no need to build a ton of support for this into the binding engine.

Thoughts? Am I way off base? I haven't been using this feature so my point of view may not match up with everyone else's.

[EisenbergEffect]

@jdanyow To make sure I understand the scenario. Is it correct to say that the only time you might want the automatic creation to happen is when you are binding from the view into the view model from a form control and part of the binding path doesn't exist?

[jdanyow]

yep

[leon-andria]

For our case, it's always on form control for editing (editor views). A display view doesn't this kind of stuff....
So, make it off and binding behavior proposal with auto:factory will make happy everyone.

[EisenbergEffect]

Our plan is to follow the recommendation of @jdanyow to remove the functionality that auto-creates instances and instead add a binding behavior that enables it. Since this will be a breaking change for some developers, this will not be released until we release Beta 2.

[jdanyow]

here's how you can disable automatic object creation in the meantime:

import {AccessMember} from 'aurelia-framework';

AccessMember.prototype.assign = function(scope, value) {
  let instance = this.object.evaluate(scope);

  if(instance === null || instance === undefined) {
    return;
  }

  return instance[this.name] = value;
}

https://gist.run/?id=2c87ea6a166cef3c1e0e

[akircher]

Thanks to all. Sounds like a good plan. In the mean time, another workaround is using something like this in the template value.bind="(nullOrObject || {}).property"

[leon-andria]

@EisenbergEffect,
If you remove the auto-creates instances by default, it will make many of us really sad.
I believe that to disable by default is a better approach.
For a community adoption, it would be good that you do a Uservoice to know the most demand of developpers.
We know that nothing is perfect but when you have to deal with many big applications to maintain, we are not going to play around breaking changes as we already did with these topic after each releases.

regards,

[EisenbergEffect]

Isn't that what we said? Auto-create would be disabled by default but you could use a binding behavior to enable it on a particular binding. Am I misunderstanding?

[leon-andria]

@EisenbergEffect,
Sorry for my bad sentence about "I believe that to disable by default is a better approach., stupid french translation :-)
What i mean is that there are more people who expect the auto-creates and disabling the option will be done explicitly by the developper.
It would be great to know the ratio of who like auto vs disable creation of instances.
I'm not "objectif" of this topic because i vote for Auto-creation enabled due to our Applications.

[EisenbergEffect]

@leon-andria I think this does relate to an issue where developers are exhibiting a bad practice by not defining their models. I'm not sure if we want to encourage that. As you can see, in this case, supporting that practice by default causes bugs in other places.

[leon-andria]

@EisenbergEffect,
I'm not fully agree with you but I accept that you are saying that we do a bad practice.

[cknightdevelopment]

I am in favor of what @EisenbergEffect is saying regarding this issue. However, to help make it easier for those that are making use of the auto-created instances heavily (bad practice or otherwise), would it be possible to let developers set the default behavior they want to have globally and then override where needed. This would allow developers to say whether or not they want the default behavior to be to auto-create instances or not. If they do not set this global setting explicitly then the default would be to not auto-create instances. Something like this:

Auto-Create Instances Global Setting	Override
N/A, default to false (DO NOT auto-create instances)	Override when you want to have auto-creation for a binding
False, DO NOT auto-created instances	Override when you want to have auto-creation for a binding
True, auto-created instances	Override when you DO NOT want to have auto-creation for a binding

This would encourage developers to not use the bad practice of not defining their models. However, if they really want to have auto-created instances then they can explicitly say that they want this behavior via the global setting. I think with this both sides of the fence would be happy, and minimal changes would be needed by the developers. Thoughts?

NOTE: I have no idea whether or not this implementation in feasible or the level of effort required from the framework point of view.

[EisenbergEffect]

It's possible. I would want to defer to @jdanyow If we did do this, we need to make it clear that there are consequences of changing the default behavior (which would be to NOT create). For example, changing that could cause 3rd party plugins to malfunction.

[Vaccano]

@EisenbergEffect - Now that you are going directly to an RC (instead of beta2) can this breaking change still be done?

I really like how @cknightdevelopment described a possibile solution.

Auto making objects leads to bugs with libraries that expect a well formed object or a null object. (For example, BreeseJs wants entities with entityAspect defined on it. A null is ok too. But a half formed object is bad and errors out.)

[atsu85]

As @jdanyow mentioned

Aurelia's behavior is not consistent, for example, value.bind="undefinedObject.someProperty" does not work the same as value.bind="undefinedObject['someProperty']". The former has magic object creation and the latter does not.

So I guess whatever will be done here, there will be a breaking change, and I think it is not a bad thing if implemented correctly (looks like most of participants agree with @jdanyow and @EisenbergEffect, so I'm not much worried about this change).

However I have one concern regarding how easily this breaking change could be adopted. If automatic object creation can be turned off ONLY globally, then i guess there might occur issues with plugins. Some of them might rely on this feature to be turned on, others might expect it to be turned off. @jdanyow, would it be possible to configure this feature separately for each plugin and the rest of the application? Then after this breaking change (for some applications and plugins), old plugins could still be used after just configuration change (if the plugin relies on auto-creating objects).

[jdanyow]

I recommend making the code change I shared above to insulate your projects from the issue. Plugin authors should not rely on the auto-object behavior- it causes bugs/confusion when used with if binding.

If there's a real, and compelling scenario where a mixed mode would be important we'd definitely want to take a look at what that would take, otherwise let's keep this as simple and predictable as possible. Goal is to avoid further issues/confusion while minimizing impact to projects that rely heavily on this behavior.

Sound reasonable?

[atsu85]

I recommend making the code change I shared above to insulate your projects from the issue.

@jdanyow, It wasn't 100% clear to me if the code chenge will be available in next release as well, or You just suggest it for those who need to disable automatic object creation?

[Vaccano]

@jdanyow, @EisenbergEffect - I ran into this issue again today. I assume there is still no way to turn this off by configuration?

Is the code listed above by @jdanyow still the accepted way to work around this?

[fkleuver]

Will be solved in vNext via the BindingFlags.