Don't export methods from panzoom directive by means of the model
Closed this issue · 11 comments
The directive currently exposes functionality by attaching methods to the model object that it receives. The five methods are:
changeZoomLevel(newZoomLevel, clickPoint)
zoomIn(clickPoint)
zoomOut(clickPoint)
getViewPosition(modelPosition)
getModelPosition(viewPosition)
The surrounding controller can then use these methods as a handy way to manipulate the model and to perform calculations. Also, the panzoomwidget directive uses them. All of the methods rely upon complex calculations which there is no reason that the surrounding controller should deal with.
From an angular perspective it seems like a hack to publish an API in this manner. However, it's also a very simple way to do so. Investigate if angular offers an equally simple way of exposing an API without offending the angular gods.
Proposal: Use a service for exposing the API (i.e. the methods above). Keep using the model as angular intends a model to be used, i.e. for maintaining and sharing state.
Current Functionality
This section briefly summarizes how things currently work between the panzoom directive and the panzoomwidget directive:
HTML:
<panzoomwidget config="panzoomConfig" model="panzoomModel"></panzoomwidget>
<panzoom config="panzoomConfig" model="panzoomModel">
<!-- content -->
</panzoom>
Enclosing controller:
$scope.panzoomConfig = {};
$scope.panzoomModel = {}; // shared model, initially empty, will be enhanced with methods
The point being that the model is how the panzoom directive exposes its API and how the panzoomwidget directive is subsequently able to use it. Simple but definitely not kosher.
Altered functionality
HTML:
<panzoomwidget panzoom-id="ThePanZoomWidget"></panzoomwidget>
<panzoom id="ThePanzoomWidget" config="panzoomConfig" model="panzoomModel">
<!-- content -->
</panzoom>
Enclosing controller:
$scope.panzoomConfig = {};
$scope.panzoomModel = {}; // model, initially empty, will *not* be enhanced with methods
Note that instead of passing the configuration and model objects to the panzoomwidget directive, it is instead passed a reference which ties it to the panzoom directive.
Behind the scenes
A PanZoomService ties everything together. Both the panzoom directive and the panzoomwidget directive rely upon this service. Any client code which wishes to use the API should also depend upon this service.
This is how I suppose that this could be made to work (untested and to be regarded as pseudo code):
panzoom.js
// in the link method where the $element is accessible ...
PanZoomService.registerAPI($element.attr('id'), {
getModel: function() { return $scope.model },
getConfig: function() { return $scope.config },
changeZoomLevel: changeZoomLevel, // defined above but not exposed on $scope.model
// etc
});
panzoomwidget.js
..
scope: {
panzoomId: '='
},
controller: function(.., PanZoomService) {
PanZoomService.getAPI(panzoomId).then(function(panZoomAPI) {
// once the API is in place, it can be used. Depending upon the order in which the directives
// are defined in the markup, it may already be defined. Otherwise it will be shortly.
// Seems like a good use case for a promise, hence the "then" callback
var model = panZoomAPI.getModel(); // this used to be passed as a direct parameter
var config = panZoomAPI.getConfig(); // same goes for this one
..
$scope.zoomIn = function() { // button click event handler
panZoomAPI.zoomIn(); // used to be: $scope.model.zoomIn();
};
});
}
Anyone defining their own equivalent of the panzoomwidget controller would have to do something similar.
As for the PanZoomService, it's basically a simple registry. The only slight complexity is that it should be able to return a promise upon lookup since the panzoomwidget may be initialized before its panzoom.
It's a breaking change so it's important to make it worth the trouble. Any comments are appreciated. If we push ahead then expect it in version 1.0.
I think it makes perfect sense.
Since we can't assume any DOM-relation between the placement of panzoom and panzoomwidget, we can't express it with the require mechanism that directives provide. This seems like a solid, flexible alternative.
I'm voting for this.
Also, if we made the connection depend upon the directive require mechanism, it would only be accessible to other directives. It would make it a bit painful in the general case. Using a service as "mediator" as proposed above will make the API more globally accessible, e.g. it would easy to use from the enclosing controller.
The approach outlined above would work fine for me, though it does feel a little heavy.
I have no attachment to the following, but just to offer a couple of alternatives:
If the panzoom directive simply published the API into the parent scope, I'd have all I need -- including the flexibility to selectively pass the API into other directives as needed. It might look something like this:
<panzoom publish="theapi">...</panzoom>
// or
<div panzoom="theapi"> ... </div>
// or
<div panzoom="someobject.theapi">...</div>
That would result in the property named 'theapi' being set to the panzoom api. Normally, this would be on the parent scope, but if some ancestor scope had defined "someobject", the api reference could be then pushed arbitrarily up the scope chain.
Controllers could then invoke it at will.
// in the controller
someobject.theapi.zoomIn();
Similarly, if I had my own custom directive that needed the API, i'd do it like this
<mydirective api="someobject.theapi">...</mydirective>
The other approach is to use an event. The panzoom directive could simply broadcast an event with the api object as an argument and which includes an ID.
<panzoom id='asdf'>...</panzoom>
// would result in something like
api.id = 'asdf';
$rootScope.$broadcast( 'panzoom-api', api );
Thanks, lmonson. That is very valuable input; just what I was hoping for when I asked you. It's a key priority to avoid any overengineered and "heavy" designs. I'll give your suggestions a very thorough consideration before returning with comments.
When looking at the suggestion of publishing the "api object" to the parent scope, it sounds somewhat like an anti-pattern, or at least like something that does not comform to "the angular way to doing things".
The two main issues I see are
- Not applicable when parent scope is isolate, eg.: a ng-if directive is applied to the parent element containing the element having the panzoom directive applied.
- The "angular"-way of exchange state between controllers is through data / function invocation on services. This just seems wrong - even though it may work.
- (I know, can't count) Doesn't work if there's no immediate child/parent hierarchy of elements having panzoom and panzoomwidget directives applied.
I do however like the suggestion of mediating the api through broadcasting. I would however prefer not to broadcast an event of the API changing, but rather broadcast the events themselves, such as:
$rootScope.$broadcast( 'panzoom-zoomfactor-change', { ... zoom factor, and reference to panzoom controller ...});
I would futhermore pass the directive reference (that would be the controller of the directive, I guess?) in the event object, instead of having to look it up through an id.
That's my immediate feedback.
As you both mention, keeping the library lightweight and intuitive should be the major goal. No one will utilize an over-engineered library / set of directives. I do however believe that following the "Angular best practices" will accomplish that.
I agree with the objection against exposing API on the parent scope. It seems to have the same basic disadvantage as my original solution in being very unconventional; in addition it has the practical drawbacks which Anders lists.
As for Anders' idea on broadcasting, I had a small discussion with him IRL (disclosure: we share the same office). We agreed that his proposal did not really address the use case of publishing the API but rather addressed the use case of publishing model changes. However, the angular way of doing this is to just share the model and let the client code bind to it or watch it.
Broadcasting the API on the root scope seems like roughly equivalent in complexity to the solution which I proposed. Both solutions require some kind of registry to mediate between the panzoom directive and API client code. In my proposal a custom service would fill this need where as in lmonson's proposal, the built-in event propagation mechanism would be used. I imagine that from a client perspective they would feel pretty much equal in terms of weight. My main concern with using event publishing is one of timing; can we always assume that the event can be published late enough to make it possible for listeners to be registered, regardless of the order of elements in the DOM? I imagine that angular.element(document).ready(..) can solve this but would that always be true? In general it would be extremely important that client code does not miss the event as they get only one shot at it.
In summary, I think both approaches would work so it's a matter of using the gut feeling and picking the one which feels the most "natural". I'm still slightly in favor of using a service as a mediator. It seems to have fewer catches.
Yeah, I've seen all 3 alternatives used and they all seem distasteful to me in varying degrees. I haven't myself (yet) seen a winning pattern in the non-singleton-data-model space for angular.
FWIW there is another disadvantage to the latter two alternates I offered up: testing isn't "explicit" -- by which I mean a controller which needs access to the panzoom api probably should have an explicit dependency injected which, in turn, can be mocked out when testing the controller. Placing objects into the parent controller's scope introduces a somewhat hard-to-notice dependency (even though there are still ways to mock it). Same objection goes for event broadcasting.
Agreed. We should try to stay clear of implicit dependencies, both for "readability" and testability.
I guess that if any one solution had initially struck me as blindingly obvious I would not even have bothered to open a discussion. By now I feel a lot more certain that I didn't overlook any blindingly obvious solution :)
lmonson, thanks for taking the time to provide your feedback. It's very much appreciated.
I expect to proceed with the service based solution once we have released v0.9.
Implemented the service based solution and released as version 1.0.0 on bower.