(1) Your are suggesting that we reverse the way we are naming things, but what you are suggesting is backwards. “Service” should be a high-level thing that sweeps some details under the rug as a convenience for the caller. “Resource” should be lower-level thing that that matches the REST resource. The reason that things are named the way there are currently is that the standard AngularJS way is to not call something “XyzResource” but rather just “Xyz”. E.g. if you look at the Credit Card Resource example at https://docs.angularjs.org/api/ngResource/service/$resource you will see
var CreditCard = $resource(...);
In our existing angular resources we have followed that pattern, e.g. EncounterType = $resource(...);. And we have and EncounterTypeService that tries to provide a nicer API on it, though we got the pattern wrong there. I could live with renaming “EncounterType” to “EncounterTypeResource” but switching the names is not cool.
(2) We should not try to abstract away the fact that the resource is built based on the idea of pages.
The way I would approach it is:
EncounterType (or EncounterTypeResource if you prefer) is a raw $resource(…).
Therefore (because this is how ngResource works) if you do EncounterType.query(…) you get back an object that is eventually filled like { results: [], links: [] } and it also has a $promise property.
That’s all, we just use the default behavior here
EncounterTypeService can introduce some smarter behavior, but we shouldn’t try to be too smart. My suggestion would to have a function named EncounterTypeService.query() that calls the underlying EncounterType.query() but instead:
it returns an array rather than an object (i.e. response.results, not response)
it returns immediately with an empty array (just like $resource does), which is soon filled by the first page of results (but not automatically by the second one)
the response array also has a $promise property on it, that you could trigger off of
the response array has methods like hasMorePages(), loadNextPage(), loadAllPages(), and these will add more results to the originally-returned array
Something like this:
var ObsServiceQueryFunction = function(query) {
var ret = [];
var underlying = ObsService.query(query)
ret.$context = /* stores what page we are up to, and what the query would be to fetch another page */;
ret.$promise = underlying.$promise.then(function(response) {
/* add everything in response.results to ret */;
});
ret.hasMoreResults = function() {
return underlying && underlying.links && _.findWhere(underlying.links, { rel: 'next' });
/* or preferrably do this based on ret.$context */
}
ret.loadNextPage = function() {
// implementation depends on how we implement ret.$context
// should add another page of results to the end of ret
// should return an array for just the newly-loaded page of results, with a $promise property
}
ret.loadAllPages = function() {
// should recursively call loadNextPage
// should progressively add pages of results to the end of ret
// I don't know what this should return, but it should definitely have a $promise on it
}
return ret;
}
I presume that we can write a utility function that implements this service query() function given any $resource as input.
In the code snippet i added to the ticket, i suggested that we expose what you are calling the service as the rest resource, kind of like a wrapper for the actual underlying rest resource, to the caller the naming convention in angular would be the same because they would still make calls the same way, basically we wouldn’t be breaking the naming convention under the hood which i think is not that bad. It makes the Resources testable and the caller doesn’t even need to know how the rest api works, i would dislike to have to do this:
Our pagination should be uniformly implement such that, If we need a convenience function to load all pages for a resource, it could done via a global utility rather than a feature duplicated within resource of each service, right?
As the original author of some of these service (I think) I will just chime in that I agree iterating/improving the design to handle pagination. I agree with Darius that we shouldn’t completely hide pagination and fetch all results by default.
@burke, yes, I envision that we have a couple utility functions that are can (a) add common implementations of retire, unretire, query, etc, to each XyzService, and (b) take a response returned by $resource and return our own standardized object with paging functions, etc.
Also, before we rush into writing a bunch of code here, someone should investigate Restangular which claims to be better than ngResource in various ways (and I think @rcrichton recommended it, based on their findings in OpenHIE).
@wyclif, if we’re going down the ngResource path, to introduce a “nice” unretire function we would do something like this:
var ProviderService = {
// ...
unretire: function(objectOrUuid) {
// one-liner that delegates to Provider/ProviderResource, which is a $resource(...)
}
}
The point is that the Provider/ProviderResource is the raw underlying implementation, and ProviderService is our nicer utility that is easier to use. It is incorrect and misleading to call the underlying resource “EncounterTypeService” and to call our nicer convenience methods “EncounterType”, as you propose in your first comment on RA-900. (I’m not really sure if we are disagreeing about anything other than naming here; I’m presuming that the “unretire” function is on the second factory in the js file.)
Generally, my takeaway from the initial implementation is that it is a mistake to try to pretend that the underlying REST behavior isn’t there, and we shouldn’t try to hide it. It seems to me that you are trying to name things in a way that hides the underlying behavior, which is only going to confuse things in the long run.
I guess i agree we can have the nicer functions in the ‘services’, i also agree that we shouldn’t completely hide pagination and always fetch all. Much as i see others agree with switching to Restangular, i don’t think this is the correct time to make the switch because i don’t think we have the time, may be in 2.4. I doubt that adding retire, unretire, purge, service functions is required as such, they are nice to have but we don’t have to add them since anyways the existing code works without them. We could add 1 or 2 just to demonstrate the desired pattern.
My concern is that the current Admin UI pages we are adding have a known
bug where if there are more than 100 of some metadata (or whatever the
usual max page size is) then you just see the first 100, with nothing else
to do. I don’t really like releasing with this known issue (though others
may disagree).
One possibility, that avoids having to quickly decide on a new service
pattern is to add a quick-hack utility function that is capable of taking a
$resource(...) object and fetching all results, and add this as a
one-liner to all the current Admin UI pages before we release.
That said, does someone want to do a quick spike of what it would look like
to reimplement a js service using Restangular instead of ngResource? It
would be helpful to see what this would look like, and if it’s really
better, given our REST implementation.
Fetching all results in angular was an exception, i agree we need to having something working by the time we release the module, it is the other things like adding service methods for retire, unretire etc and switching to Restangular that we can put off for the next release
