fix($templateRequest): always return the template that is stored in the cache

[frederikprijck]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix

What is the current behavior? (You can also link to an open issue here)
Currently, $templateRequestdoes not return the cached version returned by $templateCache. Instead, it returns the response retrieved from $http. This makes it impossible to decorate $templateCache.put as described in #16225 .

What is the new behavior (if this is a feature change)?
This commit ensures the cached value is returned instead of the $http response, allowing for proper decorating.

Does this PR introduce a breaking change?
Yes

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Fix/Feature: Docs have been added/updated
• Fix/Feature: Tests have been added; existing tests pass

Other information:
#16225

[frederikprijck]

Fix/Feature: Docs have been added/updated

Just to be sure: doc's don't need to be updated for this PR, right ?
Afaik this doesn't change anything unless when someone's already decorating the $templateCache.put method (which is what makes it a BC).

I'm not sure if this really feels like a new featurebtw (as labeled on #16225). What it does is allowing for proper decorating $templateCache.put , which wasn't possible previously. Sounds more like a "bug" fix to me, even tho it's not 100% a bug (but a bug fix sounds more logical than a new feature, imho)

[petebacondarwin]

@frederikprijck - thanks once again for helping out with these fixes. I actually agree with you that this is really a bug fix (and arguably not a breaking change). At the moment it is inconsistent.

Assuming that you have decorated $templateCache.put to transform the cached template then:

• first time you request a template, you get the raw HTTP data,
• second time you request the same template, you get the transformed cached value.

So, I would be happy to mark this as bug fix; but to be conservative we can mark it as BC and release in 1.7.0.

[petebacondarwin]

I would really like to see a unit test for this that actually does decorate the $templateCache.put method and shows that this is no longer broken, though.

[petebacondarwin]

For the commit message I would change it to the following, thinking about people who are searching for this in the changelog (not the $ in $templateRequest):

fix($templateRequest): always return the template that is stored in the cache

Previously, `$templateRequest` returned the raw `$http` response data on the
first request for a template and then the value from the cache for subsequent
requests.

If the value is transformed when being added to the cache (by decorating
`$templateCache.put`)  the return value of `$templateRequest` would be
inconsistent depending upon when the request is made.

This commit ensures the cached value is returned instead of the raw `$http`
response data, thus allowing the `$templateCache` service to be decorated.

Closes ##16225

[frederikprijck]

@petebacondarwin Regarding the test, would you really like a test that actually decorates it? Are there any other tests that actually do this kind of stuff?

What about a test like this:

it('should return the cached value on the first request',
    inject(function($rootScope, $templateRequest, $templateCache, $httpBackend) {

      $httpBackend.expectGET('tpl.html').respond('matias');
      spyOn($templateCache, 'put').and.returnValue('_matias');

      var content = [];
      function tplRequestCb(html) {
        content.push(html);
      }

      $templateRequest('tpl.html').then(tplRequestCb);
      $rootScope.$digest();
      $httpBackend.flush();

      expect(content[0]).toBe('_matias');
    }));

I'm not sure a test which actually decorates this would give any added value.

Regarding bug vs feature, might be a good idea to re-label #16225 as well.

[petebacondarwin]

This test looks fine to me :-) Thanks.

[Narretz]

Since this is now not a breaking change, should it go into 1.6.x too?

[frederikprijck]

@Narretz Iirc this still is a BC... (This probably won't affect most users, but it will affect those already decorating the put call as mentioned #16434 (comment) )

[Narretz]

Then there should have been a BC notice in the commit message :/

[frederikprijck]

I'm going to let @gkalpak / @petebacondarwin reply to that. But apparently it's a fix, not a BC. I got a bit confused on the comments 🙈

[petebacondarwin]

It is a fix not a feature.

I forgot to add a BC notice to the commit that I recommend to @frederikprijck - sorry for that - although one could argue that if you are decorating put then you should stick to the API documentation, which says that it returns the stored value. If you change the method not to do that then you are breaking the contract and that is a bug in your code.

Putting it in 1.7.0 means that even if it did break someone's code then they would be aware that changes might need to be made and so would be more consciously testing rather than doing an automatic update, which might happen with 1.6.x updates.

Let's add an issue to include a BC notice in the CHANGELOG when we release 1.7.0.