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

refactor($templateRequest): avoid double calls to $templateCache.put#10265

Closed
pkozlowski-opensource wants to merge 1 commit into
angular:masterfrom
pkozlowski-opensource:tplRequest_cache_put
Closed

refactor($templateRequest): avoid double calls to $templateCache.put#10265
pkozlowski-opensource wants to merge 1 commit into
angular:masterfrom
pkozlowski-opensource:tplRequest_cache_put

Conversation

@pkozlowski-opensource
Copy link
Copy Markdown
Member

@pkozlowski-opensource pkozlowski-opensource commented Nov 29, 2014

Passing a $templateCache instance as part of the config (as here is enough to have the cache filled correctly, we don't need explicit calls to $templateCache.put.

We've got a test that proves that things work correctly after this change:

angular.js/test/ng/templateRequestSpec.js

Lines 19 to 29 in d0351c4

it('should cache the request using $templateCache to prevent extra downloads',
inject(function($rootScope, $templateRequest, $templateCache) {
$templateCache.put('tpl.html', 'matias');
var content;
$templateRequest('tpl.html').then(function(html) { content = html; });
$rootScope.$digest();
expect(content).toBe('matias');
}));

@googlebot
Copy link
Copy Markdown

googlebot commented Nov 29, 2014

CLAs look good, thanks!

@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Dec 1, 2014

The changes look good, but the test you refer to does not prove this works. In the test, we are putting the template into $templateCache manually, so it doesn't prove that the cache is actually populated by $templateRequest.

I think a new test is needed :)

@lgalfaso
Copy link
Copy Markdown
Contributor

lgalfaso commented Dec 1, 2014

I agree with @gkalpak that a new test is needed

Somehow unrelated, what the test you point checks and it's description does not match

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Dec 1, 2014

@gkalpak @lgalfaso you are totally right, thnx for catching this one! I will push an updated version with another test. Scheduling it for 1.3.6 as it should be trivial to review.

@pkozlowski-opensource pkozlowski-opensource added this to the 1.3.6 milestone Dec 1, 2014
@pkozlowski-opensource pkozlowski-opensource self-assigned this Dec 1, 2014
@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Dec 3, 2014

@gkalpak @lgalfaso I've re-worked the test to better reflect how people are using this service. The positive side effect of this change is that we can get rid of the $templateCache dependency in tests thus hiding implementation details of the $http cache.

@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Dec 4, 2014

It looks good (to me).
A general remark: Wouldn't it make sense to verify there are no outstanding expectations/requests after every $templateRequest test (just is case) ?

@lgalfaso
Copy link
Copy Markdown
Contributor

lgalfaso commented Dec 4, 2014

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@pkozlowski-opensource pkozlowski-opensource

Labels

cla: yes

Projects

None yet

Milestone

1.3.6

Development

Successfully merging this pull request may close these issues.

4 participants

@pkozlowski-opensource @googlebot @gkalpak @lgalfaso