Removal of @Controller

[vicb]

This PR supersedes #1040.

This PR contains:

• Removal of the @Controller annotation,
• No more implicit map dereference (a.b always evaluate to a.b instead of a[b]),
• Removal of the usage of the probe directive (which is deprecated),
• Publish NgForm instance on the context
• @Template replaces @Decorator(Directive.TRANSCLUDE_CHILDREN)

see commit comments for more details

[vicb]

I've just added the publishing of NgForm instances on the context /cc @matsko

[vicb]

Updated to include #1247

[rkirov]

This line doesn't conform to DI 2.0, needs s/(_)/()/. OOC, why is this change necessary?

[vicb]

Good catch, thanks. This should be a merge artifact, I'll revert

[rkirov]

This is quite a PR, to be able to land in gracefully for internal clients we might need to reorder the features and the commits. Here is my proposed order:

• non-breaking and no new api introducing internal refactoring and polish
• add rootContextType
• remove Controller directive
• add Template directive
• remove TRANSCLUDE_CHILDREN
• Remove implicit map
• ngForm changes
• deprecate probe directive
• default context change (keeping publishAs)
• remove publishAs

At each step I would sync with clients and make sure there are no breakages. @vicb Is it feasible to create commits in that order with those features, or I am missing some dependency?

[vicb]

No this is not feasible. I have already rebased this PR several times and I think the order / content of the commits is quite optimal. Find more details below:

• non-breaking and no new api introducing internal refactoring and polish

There are a few commits with refactoring. They are placed at specified places because they depend on former changes

• add rootContextType
• remove Controller directive

I don't see a reason to separate those, they are linked.

• add Template directive
• remove TRANSCLUDE_CHILDREN

I've tried to come with something that would allow an easier transition but it would require a lot of code. In think it doesn't make a lot of sense to separate those, the transition is very easy and straightforward.

• Remove implicit map

I have to verify if this can be separated from the two first commits and update if it can not (there might be some issues with former squash / fixup)

• ngForm changes

This should be grouped with other commits as the first two commits remove the possibility to publish the form in the context that this commit re-add. If they are separated this feature will be disabled until this commit is merged.

• deprecate probe directive

Nothing is deprecated by this PR. The probe directive has been deprecated by a former commit. This PR only updates the tests to not use it any more - nothing required for client code.

• default context change (keeping publishAs)
• remove publishAs

keeping publishAs would require adding extra code in the parser, not exactly simple.

My suggestion would be to merge:

• refactor(DirectiveInjecto)
• refactor: Drop the Controller directive
• feat(form): publish NgForm instances in the context

(-> no more publishAs after this chunk)

then (I have to check if this is possible yet, if not update)

• feat: Removing implicit map dereference in expressions

then (those 3 commits could be merged separately, they do not affect client code)

• feature(ngProbe): Allow specifying a root Node when using a selector
• refactor(ngProbe) Remove usage of the probe directive (deprecated)
• style: code cleanup

then

• feat(Template): @template replaces @decorator(Directive.TRANSCLUDE_CH…

Of course there is some room for discussion but I'd rather write a migration guide and/or help the clients to migrate rather than doing some of your proposed updates (and they would take time and require having to rebase again, ... The original PR, 1040 is dated from May 13).

The migration for each of the steps I propose is quite easy and already documented in the commit comments.

Let's chat about this tomorrow or Friday.

[vicb]

@rkirov I've split the first commit in two parts:

• feat(Context): Add ability to set the Type for the rootScope context
• refactor: Drop the Controller directive …

Despite what GH prints the order is as above, it is probably related to the commit dates ?

Next, I'll:

• Merge feat(form): publish NgForm instances in the context into refactor: Drop the Controller directive …
• Update the resulting commit to ignore publishAs (instead of throwing an error) and add a commit that reverts this change.

[rkirov]

Great, thanks @vicb! I am working on landing rootContextType and removing all Controllers from clients.

@jbdeboer brought a valid point that we have no strategy for moving users of scope.context without going through controller, but once I have your commit (and no more Controllers) I can try to estimate the impact.

[vicb]

@rkirov I've implemented the changes we've talked about yesterday:

• I've re-added a publishAs field in cmp config (though it has no effect),
• I've merge this + the form publishing in Drop the controller ...

I've published my branch as it was before the changes for when w need to revert the features added in order to ease the migration.

[rkirov]

I moved clients away from Controller and probe directive (they all use ngRoute, so we can even remove the directive altogether). Next step is getting ScopeAware in, so I can more them away from injecting Scope, before landing the 'Drop the Controller directive' commit. Take a look at #1360 for that.

Also, while testing clients I noticed that injecting Scope into Components results in infinite recursion (+ stack overflow). Can we detect the infinite loop in the DirectiveInjector and throw a more informative error?

[vicb]

I moved clients away from Controller and probe directive

Great !

Also, while testing clients I noticed that injecting Scope into Components results in infinite recursion (+ stack overflow). Can we detect the infinite loop in the DirectiveInjector and throw a more informative error?

@rkirov that's probably something we should do. The directive injector was first not able to detect circular deps, the status is still not clear to me, see #1240

[vicb]

@rkirov do you agree with the following plan:

1. I wait for the circular dependency detection to be into master
2. I rebase this PR on master at that point
3. I add a separate commit to warn about ScopeAware when a circular dep is detected on creating a cmp
4. I squash the commit once you've validated it.

I think we already agreed on 1, 3 & 4. The opened question is about rebasing (2)

[rkirov]

That sounds good, can you LGTM #1364 if you think it is ready to be submitted.

[rkirov]

@vicb It's been awhile but things are moving in the right direction. I rebased the bottom two commits and reverted some of the temporary changes I made in examples. Take a look at this branch:
https://github.com/rkirov/angular.dart/tree/change-context-rebase

There will be a proper PR in a few days (when I convince myself the changes are non-breaking for client if they add ScopeAware and publishAs getters).

There were two minor changes I had to make:

• the refactor(DirectiveInjector) was creating scope lazily, which is a functional change, so I pushed in into the breaking commit.
• the todo example was broken, which turned out to be a problem with

      while (object is ContextLocals) {
        var ctx = _object as ContextLocals;
        if (ctx.hasProperty(field)) {
          _mode =  _MODE_MAP_FIELD_;
          _getter = null;
          return;
        }
        object = ctx.parentContext;
      }

I had to replace the local object with the field _object to fix it. Again take a look at my branch.

[vicb]

      while (object is ContextLocals) {
        var ctx = _object as ContextLocals;
        if (ctx.hasProperty(field)) {
          _mode =  _MODE_MAP_FIELD_;
          _getter = null;
          return;
        }
        object = ctx.parentContext;
      }

This looks wrong to me, I'll take a look at your branch

[rkirov]

Take a look at #1429. I added two tests to dirty_change_detection_spec that were failing before my modification. Basically, if we change local variable object in the setter method, we need to update the field _object so that the check method works with the same object.

[naomiblack]

Closing this PR to clean up the queue. We've been importing the work in smaller chunks. @vicb please keep the source around at your end, but we'll create new PRs moving forward.