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

bindToController with object hash allows binding properties with new instead of isolate scope #13658

Closed
Narretz opened this issue Dec 31, 2015 · 5 comments

Comments

@Narretz
Copy link
Contributor

Narretz commented Dec 31, 2015

In 35498d7, bindToController was changed to allow setting an object hash instead of true.

The object hash works the same as with the scope property, and sets up the bindings to the controller. What it not does is create an isolate scope for the directive. You have to manually set the scope property to {}, but you can also set it to true or if you don't specify it, now scope is created at all. The commit says this is intentional, but I can't see how this is something that should be possible. Using scope with a has always creates an isolate scope, and I can't see why you'd want bindings with a new / no scope. It also defeats the purpose of defining your bindings with bindToController, because you still have to set the scope prop.

The whole behavior is actually undocumented, so it's possible not many people are using it. But imo, we should change it so that bindToController with hash creates an isolate scope in all cases.

@drpicox
Copy link
Contributor

drpicox commented Jan 2, 2016

-1
Not agreee.

It should not use scope:{} in all cases. For example:

<my-component-foo data-foo="foo" my-decorator-bar data-bar="bar">
   ...

Which in both directives you can have bindToController: if decorators have scope:true can coexist with components with scope:{}.
By thumbrule I usually give scope:{} to entity directives and scope:true to attribute directives.

If you force to have always scope:{} you are actually forbidding to use bindToController in attribute directives.

@fesor
Copy link

fesor commented Jan 2, 2016

I completely agree with @drpicox. bindToController is not related to scope isolation.

@Narretz
Copy link
Contributor Author

Narretz commented Jan 4, 2016

I see what you mean. Well, we need better docs for this, then.

@gkalpak
Copy link
Member

gkalpak commented Jan 5, 2016

I too think that bindToController with scope: true is a valid usecase (albeit not a common one).
In that sense, the implementation is correct, but the docs can always be improved.

@Narretz
Copy link
Contributor Author

Narretz commented Jan 5, 2016

@gkalpak There were no docs for bindToController with object hash, that's how I stumbled upon this. Docs are here now: #13681 Can you please review? The thing is that this makes the API even more more complex, but maybe that's how it should be.

Narretz added a commit to Narretz/angular.js that referenced this issue Jan 5, 2016
Narretz added a commit to Narretz/angular.js that referenced this issue Jan 6, 2016
@Narretz Narretz closed this as completed in 495d40d Jan 6, 2016
Narretz added a commit that referenced this issue Jan 6, 2016
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

4 participants