Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

feat(ScopeAware): Introduce ScopeAware abstract class. #1360

Closed
wants to merge 1 commit into from

Conversation

rkirov
Copy link
Contributor

@rkirov rkirov commented Aug 15, 2014

Components that implement the ScopeAware scope setter, will get scope
set during compilation.

Currently, there is little incentive to use this, because Scope can be
injected in the Components. However, after PR/1269 Scope would not be
injectable and this would be the only way for components to obtain
scope.

Introducing this API earlier allows for clients to start using it and
not break when PR/1269 lands.

@rkirov rkirov mentioned this pull request Aug 15, 2014
Components that implement the ScopeAware scope setter, will get scope
set during compilation.

Currently, there is little incentive to use this, because Scope can be
injected in the Components. However, after PR/1269 Scope would not be
injectable and this would be the only way for components to obtain
scope.

Introducing this API earlier allows for clients to start using it and
not break when PR/1269 lands.
@mhevery
Copy link
Contributor

mhevery commented Aug 15, 2014

Thanks for your contribution! In order for us to be able to accept it, we ask you to sign our CLA (contributor's license agreement). CLA is important for us to be able to avoid legal troubles down the road.

For individuals (a simple click-through form): http://code.google.com/legal/individual-cla-v1.0.html

@mhevery mhevery added cla: no and removed cla: yes labels Aug 15, 2014
@vicb
Copy link
Contributor

vicb commented Aug 16, 2014

LGTM

@vicb
Copy link
Contributor

vicb commented Aug 18, 2014

As #1364 add circular dep detection, it would be easy now to wrap the component creation in a try-catch and display a warning if a circular dependency is detected.
I'm not sure if we can reliably detect whether the Scope is injected - we would have to introspect the arguments which is not possible when the code is compiled to JS plus it could be a transitive dependency.
However a (transitive) dep on Scope is probably the only possible source of exceptions and we can display a message accordingly, referring to ScopeAware.

@rkirov
Copy link
Contributor Author

rkirov commented Aug 18, 2014

Yes, as @vicb said the circular detection is the right way to do this because otherwise we would miss transitive deps (Component needs Foo, Foo needs Scope).

@vicb That's a nice idea, once #1364 you can modify it to mention "ScopeAware" when Scope is detected as part of the repeated loop. That would be much more user-friendly.

I am going to go ahead and merge this commit, as it doesn't introduce the Scope injection problem, but only the solution - ScopeAware, so that I can start moving clients to it.

@rkirov rkirov closed this in 181f014 Aug 19, 2014
rkirov added a commit that referenced this pull request Aug 19, 2014
Components that implement the ScopeAware scope setter, will get scope
set during compilation.

Currently, there is little incentive to use this, because Scope can be
injected in the Components. However, after PR/1269 Scope would not be
injectable and this would be the only way for components to obtain
scope.

Introducing this API earlier allows for clients to start using it and
not break when PR/1269 lands.

Closes #1360
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

4 participants