Skip to content
This repository was archived by the owner on Dec 1, 2023. It is now read-only.

Conversation

@guyklainer
Copy link

The issue is that the same template was loaded as mush as the directive was used.

My assumption is that the templateCache service didn't had a chance to save the template from the first fetch so he keep sending http requests each time.

solution:
save array of the promises and just if the template key is not in the array, fetch it.

@mgcrea
Copy link
Owner

mgcrea commented Nov 19, 2014

I'd rather keep using the $templateCache, could you write an unit test to reproduce the issue? (Define a custom template, intercept the http, compile another directive, check that no http req is sent).

@guyklainer
Copy link
Author

Its still using the $templateCache.
The only different is that the 'templates' object keeping the promise from the $http, so if there is one in this object, it will not call again to the $http.
The 'templates' object is just a pipe.

This fix should resolve this open issue:
#758

The solution he is using there is just a hack.

@vmlf01
Copy link
Collaborator

vmlf01 commented Nov 19, 2014

Since this is used in multiple places, maybe refactor it into a helper service?
Also, you should remove the element from templates once a template promise is resolved to cleanup.

@guyklainer
Copy link
Author

yes,
this two was exactly my next steps..

@vmlf01
Copy link
Collaborator

vmlf01 commented Nov 19, 2014

Ah, sorry, looks like you could just keep the element in the templates array to avoid going into $q.when again.

Just saw similar usage yesterday from awesome $q presentation at ng-europe: http://youtu.be/33kl0iQByME?t=13m54s

@guyklainer
Copy link
Author

Nice..
so just refactoring out to helper service... :)

@mgcrea
Copy link
Owner

mgcrea commented Nov 20, 2014

I'm not sure you need an array/map to begin with, that's redundant to what $cacheFactory is doing. To me, the proper fix should be something like this:

return $q.when($templateCache.get(template) || $http.get(template).then(putTheResponseIntoTemplateCache))

@vmlf01
Copy link
Collaborator

vmlf01 commented Nov 20, 2014

The array is just for storing the promises, not the templates themselves, so if the second request is made before the first promise fulfills, it won't trigger the $http loading of the template, it just returns the previous stored promise, and when the original $http request finishes, all the pending requests will resolve.
It is worth checking the video, I think it explains it better.

@mgcrea
Copy link
Owner

mgcrea commented Nov 20, 2014

Got it, indeed squashing requests is a good idea and that's not covered by $cacheFactory, should clearly live in a separate helper.

@mgcrea mgcrea closed this in df785dc Nov 20, 2014
@mgcrea
Copy link
Owner

mgcrea commented Nov 20, 2014

Finally implemented this without a separate helper as it would be a breaking change for any project requiring specific files of the build. Though it might be worth a refactor in the future.

@lopex
Copy link

lopex commented Nov 26, 2014

The fetchPromises cache prevents client code from updating $templateCache manually and having the new template being used from now on

@mgcrea
Copy link
Owner

mgcrea commented Nov 27, 2014

@lopex looks like this might be a bug, could you please open a new issue?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants