Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

cannot create two directives on the same element with "scope"attributes and 'bindToController:true' #10888

Closed
drpicox opened this issue Jan 27, 2015 · 4 comments

Comments

@drpicox
Copy link
Contributor

drpicox commented Jan 27, 2015

Hi,

I know that two directives cannot create a new isolated scope on the same element because of design principles (I understand and I think that such behaviour is correct).

In the Angular 1.3.X there is a new option in compiler which is bindToController:true which allows to bind attributes to the directive controller instead to the scope. In such case two directives with two different controllers can share the same scope but having its respectively attributes binded to each controller. Ex:

(In this case I have to use $attrs.$observe manually. )

What's the problem and so what I think that it can be improved: scope: { ... } is useful because it saves lot of code to syncronize and parse attributes (ex: '@' for $attrs.$observe, ...), and it is great to be reused to initialize controller attributes, but, the problem is that it also creates a new isolated scope (new and isolated), when in fact probably is not our objective (we have the controller encapsulating the state).

My proposal: if bindToController is true and scope: { ... } it does not creates a new isolated scope. In such case a new scope (but not isolated, ex scope: true, http://jsfiddle.net/drpicox/hozt6zrw/1/ ) should be the best option.

@vitaly-t
Copy link

is useful because it saves lot of code to synchronize and parse attributes

What lot of code? You can only encounter the situation when using a directive as an attribute, just like you showed in your own example, which means you've got just one property to watch within the directive.

Multiple properties are to be watched only when a directive is to be used as an element, but that means you are using your isolated scope where you're listing the supported attributes, so no issue there either.

@drpicox
Copy link
Contributor Author

drpicox commented Jan 28, 2015

This is the code saved depending which kind of scope attribute you want for each directive (code from AngularJS core):

Case '@' 2 two lines per directive (easy case), 5 lines as implemented in source code:

                attrs.$observe(attrName, function(value) {
                  isolateBindingContext[scopeName] = value;
                });
                attrs.$$observers[attrName].$$scope = scope;
                if (attrs[attrName]) {
                  // If the attribute has been provided then we trigger an interpolation to ensure
                  // the value is there for use in the link fn
                  isolateBindingContext[scopeName] = $interpolate(attrs[attrName])(scope);
                }
                break;

Case '=' is the complex case, although there are 30 lines, probably 15, but anycase let say that it will save around 10 lines for each directive plus possible bugs of forgetting unregister $watch:

                if (optional && !attrs[attrName]) {
                  return;
                }
                parentGet = $parse(attrs[attrName]);
                if (parentGet.literal) {
                  compare = equals;
                } else {
                  compare = function(a, b) { return a === b || (a !== a && b !== b); };
                }
                parentSet = parentGet.assign || function() {
                  // reset the change, or we will throw this exception on every $digest
                  lastValue = isolateBindingContext[scopeName] = parentGet(scope);
                  throw $compileMinErr('nonassign',
                      "Expression '{0}' used with directive '{1}' is non-assignable!",
                      attrs[attrName], newIsolateScopeDirective.name);
                };
                lastValue = isolateBindingContext[scopeName] = parentGet(scope);
                var parentValueWatch = function parentValueWatch(parentValue) {
                  if (!compare(parentValue, isolateBindingContext[scopeName])) {
                    // we are out of sync and need to copy
                    if (!compare(parentValue, lastValue)) {
                      // parent changed and it has precedence
                      isolateBindingContext[scopeName] = parentValue;
                    } else {
                      // if the parent can be assigned then do so
                      parentSet(scope, parentValue = isolateBindingContext[scopeName]);
                    }
                  }
                  return lastValue = parentValue;
                };
                parentValueWatch.$stateful = true;
                var unwatch;
                if (definition.collection) {
                  unwatch = scope.$watchCollection(attrs[attrName], parentValueWatch);
                } else {
                  unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal);
                }
                isolateScope.$on('$destroy', unwatch);
                break;

Case '&' saves 2 lines per directive:

                parentGet = $parse(attrs[attrName]);
                isolateBindingContext[scopeName] = function(locals) {
                  return parentGet(scope, locals);
                };
                break;

Lines save are per controller, each additional line is compressed and delivered into the browser each time that the app is loaded making heavier developments, if any user wants this behaviour the best way to deal with this situation is to implement in their own supporter object.

Of course this may be is not the right behaviour, but semantically, if we are asigning scope variables into a controller, the scope definition talks about controller, so it seems that there is a kinda lack of consistency in the directive definition since the introduction of bindToController that might be revised.

@petebacondarwin
Copy link
Contributor

I think your problem will be solved when we land: #10467

@drpicox
Copy link
Contributor Author

drpicox commented Jan 29, 2015

Yes, it indeed solves it better that I expected and in a very elegant way.
Thanks for the info.

@drpicox drpicox closed this as completed Jan 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants