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

refactor(NgView): Make use of the ViewPort #1069

Closed
wants to merge 1 commit into from
Closed

refactor(NgView): Make use of the ViewPort #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/mock/test_bed.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ part of angular.mock;
*/
class TestBed {
final Injector injector;
final Scope rootScope;
final RootScope rootScope;
final Compiler compiler;
final Parser _parser;
final Expando expando;
Expand Down
26 changes: 16 additions & 10 deletions lib/routing/ng_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ part of angular.routing;
@Decorator(
selector: 'ng-view',
module: NgView.module,
visibility: Directive.CHILDREN_VISIBILITY)
children: Directive.TRANSCLUDE_CHILDREN)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change will not work. :-( The issue is that ng-view's are nestable and so they need ChildrenVisibility, otherwise nested routes do not work. The new DirectiveInjector makes it not possible to inject Transcluded controllers. We can change the behavior, but that is the current status. Can we talk about it.

I feel like ngView should be transcluding so that it can properly handle animation, and it is what we do in AngularJS. (https://github.com/angular/angular.js/blob/master/src/ngRoute/directive/ngView.js#L188)

Copy link
Contributor

Choose a reason for hiding this comment

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

PING, can you address this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have this on my todo list, will look at it tomorrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The visibility has been removed because children visibility is the default value (Visibility.CHILDREN).

What kind of tests need to be added ? (We already have tests for nested ng-view.)

class NgView implements DetachAware, RouteProvider {
static final Module _module = new Module()
..bind(RouteProvider, toFactory: (i) => i.get(NgView));
Expand All @@ -63,17 +63,16 @@ class NgView implements DetachAware, RouteProvider {
final NgRoutingHelper locationService;
final ViewCache viewCache;
final Injector injector;
final Element element;
final Scope scope;
final ViewPort _viewPort;

RouteHandle _route;

View _view;
Scope _scope;
Route _viewRoute;

NgView(this.element, this.viewCache,
Injector injector, Router router,
this.scope)
NgView(this.viewCache, Injector injector, Router router, this.scope, this._viewPort)
: injector = injector,
locationService = injector.get(NgRoutingHelper)
{
Expand Down Expand Up @@ -119,24 +118,31 @@ class NgView implements DetachAware, RouteProvider {
viewFuture.then((viewFactory) {
_cleanUp();
_scope = scope.createChild(new PrototypeMap(scope.context));
_view = viewFactory(
viewInjector.createChild([new Module()..bind(Scope, toValue: _scope)]));
_view.nodes.forEach((elm) => element.append(elm));
_view = viewFactory(viewInjector.createChild([new Module()..bind(Scope, toValue: _scope)]));
_scope.rootScope.domWrite(() {
_viewPort.insert(_view);
});
});
}

_cleanUp() {
if (_view == null) return;

_view.nodes.forEach((node) => node.remove());
_scope.destroy();
var view = _view;
var childScope = _scope;
_scope.rootScope.domWrite(() {
_viewPort.remove(view);
childScope.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

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

destroying scope in domWrite feels odd to me... but I can't say if that will cause issues... just pointing it out... no action required.

Copy link
Contributor

Choose a reason for hiding this comment

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

destroying scope in domWrite is wrong. It should be destroyed immediately, and DOM operation delayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping

});

_view = null;
_scope = null;
}

Route get route => _viewRoute;

String get routeName => _viewRoute.name;

Map<String, String> get parameters {
var res = <String, String>{};
var p = _viewRoute;
Expand Down
28 changes: 19 additions & 9 deletions test/routing/ng_view_spec.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,57 +28,62 @@ main() {


it('should switch template', async(() {
Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you wrap these in div?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because ngview need to have a parent for transclude to work as explained in the PR comment. Let me know of anything is unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually let me try to put this in a clearer way:

Without the wrapping <div>, compile() would return the <ng-view> node.

It was perfectly fine before this PR because the nodes were added as children of <ng-view>.

Now that we use transclusion, the <ng-view> node is removed from the DOM and replaced by an anchor (<!--ANCHOR: ng-view-->). The view nodes are appended as siblings of this anchor the <ng-view> node is removed from the DOM at compile time and <ng-view>.text will always ever contain its content at compile time: "".

expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('Foo');

router.route('/bar');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('Bar');

router.route('/foo');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('Foo');
}));

it('should expose NgView as RouteProvider', async(() {
_.compile('<ng-view probe="m"></ng-view>');
_.compile('<div><ng-view probe="m"></ng-view></div>');
router.route('/foo');
microLeap();
_.rootScope.apply();
_.rootScope.flush();

expect(_.rootScope.context['p'].injector.get(RouteProvider) is NgView).toBeTruthy();
}));


it('should switch template when route is already active', async(() {
// Force the routing system to initialize.
_.compile('<ng-view></ng-view>');
_.compile('<div><ng-view></ng-view></div>');

router.route('/foo');
microLeap();
Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

_.rootScope.apply();
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('Foo');
}));


it('should clear template when route is deactivated', async(() {
Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('Foo');

router.route('/baz'); // route without a template
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('');
}));

Expand Down Expand Up @@ -111,25 +116,29 @@ main() {
});

it('should switch nested templates', async(() {
Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/library/all');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('LibraryBooks');

router.route('/library/1234');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('LibraryBook 1234');

// nothing should change here
router.route('/library/1234/overview');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('LibraryBook 1234');

// nothing should change here
router.route('/library/1234/read');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('LibraryRead Book 1234');
}));
});
Expand Down Expand Up @@ -157,11 +166,12 @@ main() {
});

it('should switch inline templates', async(() {
Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();
expect(root.text).toEqual('Hello');
}));
});
Expand Down
17 changes: 11 additions & 6 deletions test/routing/routing_spec.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,12 @@ main() {
_.injector.get(TemplateCache)
.put('foo.html', new HttpResponse(200, '<h1>Foo</h1>'));

Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();

expect(enterCount).toBe(1);
expect(root.text).toEqual('Foo');
Expand Down Expand Up @@ -231,17 +232,19 @@ main() {
_.injector.get(TemplateCache)
.put('foo.html', new HttpResponse(200, '<h1>Foo</h1>'));

Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();

expect(root.text).toEqual('Foo');
expect(leaveCount).toBe(0);

router.route('/bar');
microLeap();
_.rootScope.flush();

expect(root.text).toEqual('');
expect(leaveCount).toBe(1);
Expand All @@ -263,11 +266,12 @@ main() {
_.injector.get(TemplateCache)
.put('foo.html', new HttpResponse(200, '<div make-it-new>Old!</div>'));

Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();

expect(root.text).toEqual('New!');
}));
Expand All @@ -288,11 +292,12 @@ main() {
_.injector.get(TemplateCache)
.put('foo.html', new HttpResponse(200, '<div make-it-new>Old!</div>'));

Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
microLeap();
_.rootScope.flush();

expect(root.text).toEqual('New!');
}));
Expand All @@ -313,7 +318,7 @@ main() {
_.injector.get(TemplateCache)
.put('foo.html', new HttpResponse(200, '<div>{{\'World\' | hello}}</div>'));

Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
Expand All @@ -339,7 +344,7 @@ main() {
_.injector.get(TemplateCache)
.put('foo.html', new HttpResponse(200, '<div>{{\'World\' | hello}}</div>'));

Element root = _.compile('<ng-view></ng-view>');
Element root = _.compile('<div><ng-view></ng-view></div>');
expect(root.text).toEqual('');

router.route('/foo');
Expand Down