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

fix($templateRequest): Handle empty templates in cache correctly. #14496

Closed
wants to merge 2 commits into from
Closed

fix($templateRequest): Handle empty templates in cache correctly. #14496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
deea0fc
fix($templateRequest): Handle empty templates in cache correctly.
rjamet Apr 22, 2016
b0038a3
fix($templateRequest): Make the test more reliable, and simplify the …
rjamet Apr 22, 2016
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 src/ng/templateRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ function $TemplateRequestProvider() {
// are included in there. This also makes Angular accept any script
// directive, no matter its name. However, we still need to unwrap trusted
// types.
if (!isString(tpl) || !$templateCache.get(tpl)) {
if (!isString(tpl) || isUndefined($templateCache.get(tpl))) {
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you just change this line to ... || isUndefined($templateCache.get(tpl))... and leave everythng else unchanged ?

Copy link
Contributor Author

@rjamet rjamet Apr 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The reasoning is that people asked for wrong fixes several times, so my original code was probably confusing. I've unrolled the if to accomodate clearer comments, but that can be reverted if you prefer.

Edit : No point making this longer than needed, actually. Reverted to the simpler version.

tpl = $sce.getTrustedResourceUrl(tpl);
}

Expand Down
33 changes: 33 additions & 0 deletions test/ng/templateRequestSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,39 @@ describe('$templateRequest', function() {
}).not.toThrow();
}));

it('should accept empty templates and refuse null or undefined templates in cache',
inject(function($rootScope, $templateRequest, $templateCache, $sce) {

// Will throw on any template not in cache.
spyOn($sce, 'getTrustedResourceUrl').and.returnValue(false);

expect(function() {
$templateRequest('tpl.html'); // should go through $sce
$rootScope.$digest();
}).toThrow();

$templateCache.put('tpl.html'); // is a no-op, so $sce check as well.
expect(function() {
$templateRequest('tpl.html');
$rootScope.$digest();
}).toThrow();
$templateCache.removeAll();

$templateCache.put('tpl.html', null); // makes no sense, but it's been added, so trust it.
expect(function() {
$templateRequest('tpl.html');
$rootScope.$digest();
}).not.toThrow();
$templateCache.removeAll();

$templateCache.put('tpl.html', ''); // should work (empty template)
Copy link
Member

Choose a reason for hiding this comment

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

Since you have already called $templateCache.put() previously, this test is not so reliable.
I would either empty the cache first ot use the they helper to make each case its own testcase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good point. Fixed with deleteAll after each of the blocks.

expect(function() {
$templateRequest('tpl.html');
$rootScope.$digest();
}).not.toThrow();
$templateCache.removeAll();
}));

it('should keep track of how many requests are going on',
inject(function($rootScope, $templateRequest, $httpBackend) {

Expand Down