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

feat(ngMessagesInclude): don't break on empty (or whitespace-only) templates #14726

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
36 changes: 23 additions & 13 deletions src/ngMessages/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -552,22 +552,32 @@ angular.module('ngMessages', [])
$templateRequest(src).then(function(html) {
if ($scope.$$destroyed) return;

$compile(html)($scope, function(contents) {
element.after(contents);

// the anchor is placed for debugging purposes
var comment = $compile.$$createComment ?
$compile.$$createComment('ngMessagesInclude', src) :
$document[0].createComment(' ngMessagesInclude: ' + src + ' ');
var anchor = jqLite(comment);
element.after(anchor);

// we don't want to pollute the DOM anymore by keeping an empty directive element
element.remove();
});
if (isString(html) && !html.trim()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

trim is doing an isString check, so we should be able to skip it here

Copy link
Contributor

Choose a reason for hiding this comment

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

The isString is needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Test passes without it ...

Copy link
Member Author

@gkalpak gkalpak Jun 7, 2016

Choose a reason for hiding this comment

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

The isString() is needed in case the request returns some non-string data. This is normally not the case, but can happen (especially in tests).

I didn't test for it explicitly though...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's why I was confused. If it's not tested it's not really specified. But in that case it's not big deal

// Empty template - nothing to compile
replaceElementWithMarker(element, src);
} else {
// Non-empty template - compile and link
$compile(html)($scope, function(contents) {
element.after(contents);
replaceElementWithMarker(element, src);
});
}
});
}
};

// Helpers
function replaceElementWithMarker(element, src) {
// A comment marker is placed for debugging purposes
var comment = $compile.$$createComment ?
$compile.$$createComment('ngMessagesInclude', src) :
$document[0].createComment(' ngMessagesInclude: ' + src + ' ');
var marker = jqLite(comment);
element.after(marker);

// Don't pollute the DOM anymore by keeping an empty directive element
element.remove();
}
}])

/**
Expand Down
20 changes: 19 additions & 1 deletion test/ngMessages/messagesSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,6 @@ describe('ngMessages', function() {
expect(trim(element.text())).toEqual("C");
}));


it('should properly detect a previous message, even if it was registered later',
inject(function($compile, $rootScope, $templateCache) {
$templateCache.put('include.html', '<div ng-message="a">A</div>');
Expand Down Expand Up @@ -865,6 +864,25 @@ describe('ngMessages', function() {
$httpBackend.flush();
}).not.toThrow();
}));

it('should not throw if the template is empty',
inject(function($compile, $rootScope, $templateCache) {
var html =
'<div ng-messages="items">' +
'<div ng-messages-include="messages1.html"></div>' +
'<div ng-messages-include="messages2.html"></div>' +
'</div>';

$templateCache.put('messages1.html', '');
$templateCache.put('messages2.html', ' ');
element = $compile(html)($rootScope);
$rootScope.$digest();

expect(element.text()).toBe('');
expect(element.children().length).toBe(0);
expect(element.contents().length).toBe(2);
})
);
});

describe('when multiple', function() {
Expand Down