ability to add logging info when binding to non-existent properties

[devmondo]

the idea is that sometimes we bind to a none existent property, and the logger should tell us that this property does not exist.

but one of the great things about Aurelia is that sometimes you want to bind to dynamic properties, so making this an optional flag in logging verbosity level or something would make it better.

[jdanyow]

related to #23

[jdanyow]

maybe it would also be useful to log when dirty-checking is used in a browser with O.o support.

[jdanyow]

this came up in the gitter again today: https://gitter.im/Aurelia/Discuss?at=55140f63c25916c13d35183d

[andreister]

I tried to look into this but got stuck at adding logging to property-observation.js

I changed the code there to inject the logger (similar to how it's done in other places)

System.register(["aurelia-logging"], function (_export) {
   var LogManager, _logger, ...
    ...
   return {
    setters: [
      function(_aureliaLogging) {
        LogManager = _aureliaLogging;
      }
    ],
    ...

and updated jspm_packages\github\aurelia\[email protected]\.jspm.json to include the new dependency, but this didn't work - until I also mentioned the new dependency in my app's config.js

System.config({
  "map": {
     ...
    "github:aurelia/[email protected]": {
      "aurelia-logging": "github:aurelia/[email protected]",
       ...
    },
   ...

why is that? Am I missing anything, or it's really necessary to update the main config.js?

[jdanyow]

Hey- thanks for looking a this, you're on the right track. What you're running into is aurelia/binding doesn't have a dependency on aurelia/logging yet. You'd need to `jspm install aurelia-logging' (which would add the entries in config.js). config.js is typically maintained by jspm, usually you don't need to hand edit it.

The logging code might go higher up, in the ObserverLocater class, and should be configurable. Reason is certain observer classes are used in certain scenarios, depending on the browser's support for O.o, etc.

[andreister]

Thanks for the explanations! Didn't realize jspm install is the way to change config.js

I'm quite new to all this so will need some more guidance. Can you look at my commit https://github.com/andreister/binding/commit/f616ec88494f22bf82e9a6b42b1e7fffc6a26f28 and see if it's close?

I'm not sure about your "...should be configurable...", what kind of configuration do you mean? I see configurable: true all over the code, but cannot yet understand how it's used.

[jdanyow]

Adding property existence checks in AccessScope, AccessMember, CallScope, CallMember could be bad news for performance, especially with AccessScope's ability to find props on the ancestor scopes.

[EisenbergEffect]

Is there some way that we could have a debug mode...where when it is turned on the AST parsing substitutes different debug mode AST instances perhaps? Just thinking out loud here...

[jdanyow]

Sounds like a good idea to me. Will need to make sure the test coverage is beefed up in those areas. I think we could modify some of the existing AST tests to enable reuse between AccessScope and "DebugAccessScope" if that's the way we go about implementing this feature.

[EisenbergEffect]

Sounds good. It would be nice to have something like this. We'll have to consider how it should be configured for debug mode. We may need to put a setting on the BindingEngine and then somehow surface that through the Aurelia FrameworkConfiguration object.

[jdanyow]

The configuration piece will be important to nail down for #205 as well

[EisenbergEffect]

Good point.

[bigopon]

@jdanyow @EisenbergEffect Can we simplify it down to this line

When we try to do everything in getObserver but couldn't figure out the right observer, and the property doesn't exist in prototype chain, we log a warning ?

// Instead of
// return new SetterObserver(this.taskQueue, obj, propertyName);

// We do
this.logger
      .warn(`Attempted to observe non existing property [${propertyName}] on instance of [${obj.constructor.name}]. Fallback to SetterObserver`);
    return new SetterObserver(this.taskQueue, obj, propertyName);

[EisenbergEffect]

My only concern would be whether or not this causes a perf regression.

[bigopon]

we can add PLATFORM.isProduction property to use for this purpose, followed by changes in bundlers what do you think ? @EisenbergEffect

[Alexander-Taran]

@EisenbergEffect mentioned this one in vNext cases
this one can be closed I guess