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

directive compile function scope pollution (ngModel) #4889

Closed
lukemadera opened this issue Nov 11, 2013 · 20 comments
Closed

directive compile function scope pollution (ngModel) #4889

lukemadera opened this issue Nov 11, 2013 · 20 comments

Comments

@lukemadera
Copy link

I have a forminput directive (originally adapted from @mhevery example here: http://stackoverflow.com/questions/10629238/angularjs-customizing-the-template-within-a-directive ) that no longer works in the new Angular 1.2.0. It works in Angular 1.2.0-rc.3.

jsFiddles here:
WORKS: 1.2.0-rc.3 http://jsfiddle.net/lukem/9BpSm/
does NOT work: 1.2.0 http://jsfiddle.net/lukem/S9rV2/

I read the changelog and saw there were numerous (breaking) changes to the compile function but it's not clear to me what is causing this new behavior and if it's intended to be this way (in which case how am I correctly supposed to do it?) or if it's a bug.

It DOES seem to work with template instead of compile so perhaps it's related to d0efd5e ? @vojtajina is there a reason isolate scope doesn't also apply to compile functions as well as the template function?

@tbosch
Copy link
Contributor

tbosch commented Nov 11, 2013

Yes,
this is the intended behavior:
The isolate scope is only applied to the template, but not to markup that was not contributed by the directive. Before 1.2 isolate scopes had a bug that caused this kind of leakage. The point of the isolate scope is that it only applies to the directive that declared it, and not to other directives or markup.

As we can't distinguish markup that was originally in the html file and markup that was added in the compile function the latter also doesn't get the isolate scope.

So yes, using the template is the right and intended way now.

Tobias

@tbosch
Copy link
Contributor

tbosch commented Nov 11, 2013

Closing this as not a bug...

@tbosch tbosch closed this as completed Nov 11, 2013
@lukemadera
Copy link
Author

Thanks for the comment @tbosch but unfortunately it's still unclear to me - are you saying the 'compile' function is now obsolete in v1.2.0 (which it seems is the case for practical purposes if isolate scope can't be used with compile anymore)? If so, that should be clearly documented in the CHANGELOG, docs, etc.

Additionally, it still does not solve the problem at hand and that was referenced by @mhevery in his post http://stackoverflow.com/questions/10629238/angularjs-customizing-the-template-within-a-directive - which is - how do you build a directive's HTML/template dynamically, using the directive attrs?

The template function is great for simple/static templating/HTML, but (unless I'm mistaken), doesn't allow any access to directive attrs? We need a way to utilize these attributes when building the HTML, i.e. how would you accomplish this in 1.2.0 WITHOUT using "ng-if", etc. on the scope and cluttering the DOM (this is just a simple example - in reality my directive handles selects, textareas and other inputs that require completely different HTML markup)?

http://jsfiddle.net/lukem/S9rV2/

Could you please fork that fiddle and show how to build an input that has a dynamic 'type' in 1.2.0?

Thanks!

@lukemadera
Copy link
Author

Also @tbosch could you please not close the issue until it's actually resolved? If you feel it's not a "bug" and want to label it differently and/or feel I'm posting in the wrong forum for this please feel free to provide suggested alternatives, but this is "breaking behavior" that's not well documented so others are likely experiencing this as well.

@tbosch
Copy link
Contributor

tbosch commented Nov 12, 2013

Hi @lukemadera, sorry, I didn't mean to annoy you.

Regarding your questions:
The new behavior only applies if you are using an isolate scope, i.e. scope: { ... }. The implementation of isolate scopes was broken before but fixed now.

Then, when using a directive that uses an isolate scope (only then):
The compile function is not obsolete, only adding additional markup in the compile function should be replaced by using the template property.

The idea is that the template property can also be a function. If it is one, it will get the compile element and the compile attributes as parameters.

Here is an updated jsfiddle: http://jsfiddle.net/S9rV2/4/

Keeping the issue open this time :-)

Tobias

@tbosch tbosch reopened this Nov 12, 2013
@tbosch
Copy link
Contributor

tbosch commented Nov 12, 2013

Ah, and here is the API doc about it: http://docs.angularjs.org/api/ng.$compile

True, the changelog (https://github.com/angular/angular.js/blob/master/CHANGELOG.md) does not include your exact case, but it does include a breaking change for how isolate scopes are assigned to DOM elements.

Also in the changelog, under new features for 1.1.4 we mention that templates can now be generated dynamically.

Thanks for your response as it shows that this change wasn't clearly pointed out by us!

@evgenyneu
Copy link

@tbosch, Moving to 1.2.0 from 1.2.0-rc.3 broke my directive unit tests. I think for the same reason.

Before I could do something like that in a test

element = $compile(markup)(scope)
scope = element.scope()

to get directive's isolated scope. But now it looks like element.scope() returns a parent scope. Is there a way to get directive's scope in a unit test?

Currently I am doing it by element.scope().$$childTail, but it looks hacky.

@caitp
Copy link
Contributor

caitp commented Nov 12, 2013

@evgenyneu as of 27e9340 you can use element.isolateScope() to access its isolate scope.

To my knowledge this does not walk up the tree, however, so it needs to be used on the root of the directive.

@evgenyneu
Copy link

@caitp, element.isolateScope() is exactly what I needed! Thanks a lot, friend, you saved my day.

@lukemadera
Copy link
Author

No worries @tbosch and thanks for the fiddle - switching to the template as a function did the trick, I'm all set now! I do remember reading about the 1.1.4 change to a template function but to be honest directives are still by far the most confusing part of Angular (even though they're the most powerful) so once I had something that worked (with the compile function), I wasn't about to change it without good reason! Though after reading through the new/update docs on directives on angularjs.org I think it's much better - especially the "best practices" green boxes are very helpful. There's so many different ways to build a directive and while I know that leads to a lot of power, it also can create lots of confusion. Making progress!

Thanks again.

@lukemadera
Copy link
Author

Related question - @caitp (and/or @tbosch ) - angular.element(this).scope() is no longer working in 1.2.0 from within a directive - see fiddle here: http://jsfiddle.net/lukem/dvu7U/
(just change the script at top to toggle between 1.2.0 and 1.2.0-rc.3)

I tried .isolateScope() as well but that was undefined. I'd like to just use ng-change but that doesn't seem to be firing so this was the work-around up until 1.2.0 based on this: http://jsfiddle.net/danielzen/utp7j/

Does it have to do with the this keyword not referencing the correct thing?

Basically I need a way to determine when a user has selected a file from a file input. Any help would be appreciated!

@lukemadera lukemadera reopened this Nov 12, 2013
@caitp
Copy link
Contributor

caitp commented Nov 12, 2013

@lukemadera for starters, the context for event handlers (using the on{{eventname}} attributes) are actually the global context (which in web apps is generally window), so I think element(this).scope() would probably end up giving you $rootScope, if it gave you anything at all.

Anyways, ng-change does not wrap the change event handler. Instead, the expression is evaluated after a view change, if the model value is not equal to the previous value. (actually it might be the view value, I can't remember, but it's easy to look up). Not all interactions will necessarily cause a view change, and if you're writing a custom directive which uses ng-model, and you aren't calling $setViewValue anywhere, it is definitely not going to trigger an ng-change expression.

At any rate, if you do want to handle change, you're better off registering a listener through jqlite/jquery. This way, it can just be a closure within your directive which would have access to the isolate scope anyways.

@lukemadera
Copy link
Author

Gotcha, thanks @caitp - I got it working with jqLite .on('change'). Thanks for the help and prompt reply!

@tbosch
Copy link
Contributor

tbosch commented Nov 12, 2013

For all other people who are reading this issue:
There is a section in the migration guide about the changes to the isolate scope too: http://docs.angularjs.org/guide/migration#isolate-scope-only-exposed-to-directives-with-property

@tbosch
Copy link
Contributor

tbosch commented Nov 12, 2013

@evgenyneu Using element.isolateScope() should only be needed in very rare cases.
Could you explain your use case, I am very curious wether there is something we haven't thought...

T

@caitp
Copy link
Contributor

caitp commented Nov 12, 2013

@tbosch They have stated that 1.2.0 (and the subsequent true isolation of isolate scope) broke unit tests which looked like

element = $compile(markup)(scope)
scope = element.scope()

because previously they were able to access the isolate scope like this. I believe isolateScope() is an appropriate replacement for this strategy in tests

@tbosch
Copy link
Contributor

tbosch commented Nov 12, 2013

Ah, ok, it's about unit tests, sorry, didn't get that.
So yes, for unit tests using element.isolateScope() is fine.

@BendingBender
Copy link

@tbosch Regarding the usage of isolateScope(): We used to have directives that enhanced functionality of widgets with an isolate scope. Imagine a widget that under certain circumstances needs to become drag&dropable. Usually this widget has no idea that it is drag&dropable (D&D) but there existed an extra "marker" directive, that, when appended as attribute in widget's root element, made this widget draggable. This marker also activated certain functionality on the widget during drag. Now, how the two are going to communicate?

In older angular versions, this was easy because the extra directive would get the reference to the widget's isolate scope and could directly call methods on it.

Now, with the new version of angular, we somehow need to expose the extra api to be accessible from the outer scope. This can be done via value binding or directly by observing the attributes object on the widget's side. This means however that the widget has to be prepared for this to work and needs to do something more complex than actually needed. The other solution is to request the isolate scope in the "marker" directive to imitate the behaviour of older anguar versions.

@rodyhaddad
Copy link
Contributor

@BendingBender in order for directives to communicate, one should expose its API via a controller, and the other should require it. That's the proper way of doing it.

In the same way that you can require: 'ngModel' to get the ngModelController of the element, your widget directive should expose it's api via a controller,

.directive('enhanceWidget', function () {
  return {
    scope: {...},
    controller: function ($scope, $element, $attr) {
      this.someHook = ...
    }
  };
})

and your "marker" directive can communicate with it by requiring it's controller

.directive('marker', function () {
  return {
    require: 'enhanceWidget',
    link: function (scope, el, attr, enhanceWidget) {
      enhanceWidget.someHook(...)
    }
  };
})

Also, in the code above, marker can't be used if enhanceWidget is not present on the element (which is a plus in your case. You can play around with that (e.g. remove that restriction). It's all specified in the $compile docs)

@tbosch
Copy link
Contributor

tbosch commented Nov 21, 2013

@rodyhaddad thanks for the comment, I agree.

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

No branches or pull requests

6 participants