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

Context change #1429

Closed
wants to merge 2 commits into from
Closed

Context change #1429

wants to merge 2 commits into from

Conversation

rkirov
Copy link
Contributor

@rkirov rkirov commented Sep 5, 2014

This consists of the first two commits from #1269, rebased and slightly modified.

@rkirov rkirov mentioned this pull request Sep 5, 2014
Closed
@mhevery mhevery added cla: yes and removed cla: no labels Sep 5, 2014
@vicb
Copy link
Contributor

vicb commented Sep 5, 2014

@rkirov tests do not pass, the PR needs to be updated. For the next time it would be better to keep the original commits and the updated separated to ease the review (and squash once after the review).

@rkirov rkirov force-pushed the change-context-rebase branch from d049f72 to 600bd19 Compare September 5, 2014 21:10
@rkirov
Copy link
Contributor Author

rkirov commented Sep 5, 2014

All tests pass except G3 ones which have an unrelated merge conflict.

@rkirov rkirov force-pushed the change-context-rebase branch 2 times, most recently from 90eb785 to b981052 Compare September 8, 2014 21:58
vicb
vicb reviewed Sep 11, 2014
View reviewed changes
lib/change_detection/dirty_checking_change_detector.dart
_mode = _MODE_MAP_FIELD_;
_getter = null;
} else {
while (_object is ContextLocals) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be add a comment explaining why we work with _object here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using _object instead of object everywhere ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. Added a warning to use _object throughout setter to avoid bugs.

@vicb
Copy link
Contributor

vicb commented Sep 11, 2014

@rkirov I've added some comments and the tests still do not pass (not related to g3)

@rkirov rkirov force-pushed the change-context-rebase branch from b981052 to 8df0052 Compare September 11, 2014 23:57
rkirov added a commit to rkirov/angular.dart that referenced this pull request Sep 12, 2014
@rkirov
refactor(NgElement): NgElement inject RootScope instead of Scope.
dc6b8ab 
NgElement only uses scope to access rootScope, so now we directly inject
RootScope.

This is needed for future change dart-archive#1429 which disallows Scope injection
into controllers. Since NgElement depends on scope it would also be
disallowed, unless we change it to directly use RootScope.
@rkirov rkirov mentioned this pull request Sep 12, 2014
Closed
@rkirov rkirov force-pushed the change-context-rebase branch from 8df0052 to f556b1a Compare September 12, 2014 22:18
@rkirov
Copy link
Contributor Author

rkirov commented Sep 12, 2014

I added PrototypeMap back into exported symbols as we have clients that use it.

@rkirov rkirov force-pushed the change-context-rebase branch from f556b1a to aae2e83 Compare September 12, 2014 22:32
@rkirov rkirov force-pushed the change-context-rebase branch 2 times, most recently from 09d6e90 to 3f87448 Compare September 15, 2014 23:03
rkirov added a commit to rkirov/angular.dart that referenced this pull request Sep 22, 2014
@rkirov
refactor(NgElement): NgElement inject RootScope instead of Scope.
a952ebb 
NgElement only uses scope to access rootScope, so now we directly inject
RootScope.

This is needed for future change dart-archive#1429 which disallows Scope injection
into controllers. Since NgElement depends on scope it would also be
disallowed, unless we change it to directly use RootScope.
rkirov added a commit that referenced this pull request Sep 23, 2014
@rkirov
refactor(NgElement): NgElement inject RootScope instead of Scope.
bc21766 
NgElement only uses scope to access rootScope, so now we directly inject
RootScope.

This is needed for future change #1429 which disallows Scope injection
into controllers. Since NgElement depends on scope it would also be
disallowed, unless we change it to directly use RootScope.
rkirov added a commit that referenced this pull request Sep 23, 2014
@rkirov
refactor(NgElement): NgElement inject RootScope instead of Scope.
3983b5f 
NgElement only uses scope to access rootScope, so now we directly inject
RootScope.

This is needed for future change #1429 which disallows Scope injection
into controllers. Since NgElement depends on scope it would also be
disallowed, unless we change it to directly use RootScope.
@rkirov rkirov force-pushed the change-context-rebase branch from 3f87448 to 89fe8f2 Compare September 24, 2014 02:52
@rkirov
Copy link
Contributor Author

rkirov commented Sep 25, 2014

@vicb I had to move the while (object is ContextLocal) to avoid breaking use cases like this one:
https://gist.github.com/rkirov/55f5d16832971e2f26c2
Will extract it to a test case later, but can you check if there is a better way to solve that issue.

@vicb
Copy link
Contributor

vicb commented Sep 25, 2014

I think that the issue is that my original PR is not fully merged.
It must be due to the artificial getter that got introduced.

  1. when you say "foo" you want to access "rootContext.foo"

  2. when you say "ctrl.foo" you first access the added getter then the
    foo property on the return value of the getter.

The issue is that the meaning of the expressions is different according
to the context.

When the PR will be fully merged you will say "foo" for both 1 & 2 and
we should be able to revert to the original code
(move the while back to were it was in my PR).

Cheers,
Vic

On 09/25/2014 07:01 AM, Rado Kirov wrote:

@vicb https://github.com/vicb I had to move the |while (object is
ContextLocal)| to avoid breaking use cases like this one:
https://gist.github.com/rkirov/55f5d16832971e2f26c2
Will extract it to a test case later, but can you check if there is a
better way to solve that issue.

—
Reply to this email directly or view it on GitHub
#1429 (comment).

@rkirov rkirov force-pushed the change-context-rebase branch 3 times, most recently from 5e1a70f to e2c5d16 Compare September 30, 2014 04:53
@rkirov rkirov force-pushed the change-context-rebase branch from e2c5d16 to 2681820 Compare October 1, 2014 17:12
vicb added 2 commits October 2, 2014 10:12
@vicb @rkirov
refactor(DirectiveInjector): better var names and comments
eb81e93
@vicb @rkirov
feat(scope): component is the new context
cc294d8 
BREAKING CHANGE:

Scope context is set to the component instance that trigged the creation
of the scope (previously it was of a PrototypeMap.)

Repercussions:
1) You can not inject a scope in a component or in the root context any
more.

As the Scope context is set to the Component instance, the scope could
not be injected any more. Components should implements the "ScopeAware"
interface and declare a "scope" setter in order to get a reference to
the scope.

before:

     @component(...)
     class MyComponent {
       Watch watch;
       Scope scope;

       MyComponent(Dependency myDep, Scope scope) {
          watch = scope.rootScope.watch("expression", (v, p) => ...);
       }
     }

after:

     @component(...)
     class MyComponent implements ScopeAware {
       Watch watch;

       MyComponent(Dependency myDep) {
         // It is an error to add a Scope argument to the
         // ctor and will result in a DI circular dependency error
         // The scope is never accessible in the class constructor
       }

       void set scope(Scope scope) {
          // This setter gets called to initialize the scope
          watch = scope.watch("expression", (v, p) => ...);
       }
     }

or:

     @component(...)
     class MyComponent implements ScopeAware {
       Scope scope;

       MyComponent(Dependency myDep) {
         // It is an error to add a Scope argument to the
         // ctor and will result in a DI circular dependency error
         // The scope is never accessible in the class constructor
       }
     }

2) The parent component to an NgForm must have a "$name" field to store
   the form instance.

closes dart-archive#919
closes dart-archive#917
@rkirov rkirov force-pushed the change-context-rebase branch from 2681820 to cc294d8 Compare October 2, 2014 17:21
@mhevery
Copy link
Contributor

mhevery commented Oct 2, 2014

LGTM

@rkirov rkirov closed this Oct 2, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
action: merge api: breaking cla: yes status: LGTM status: Travis OK
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rkirov @vicb @mhevery