Bugzilla – Full Text Bug Listing
| Summary: | [client] Need manageable story for service initialization and stating service dependencies | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Boris Bokowski <bokowski> |
| Component: | Client | Assignee: | Simon Kaegi <simon_kaegi> |
| Status: | RESOLVED WONTFIX | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | john.arthorne, mamacdon, pwebster, simon_kaegi, susan |
| Version: | 0.2 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | 334197 | ||
| Bug Blocks: | |||
what's nice about passing in the services directly is that someone running outside of the "client" (such as embedded editor in firebug) could be passed a "status manager" that has nothing to do with services at all, it could just be a simple firebug implementation for notifying the user of stuff from the editor. In bug 337647, I'm starting to change the editor container to treat all these services as optional. Is there status service? report it. Is there an undo stack? then manage composite changes. I could change the constructors for things like the editor, favorites, nav, etc. to start using this pattern, but I would want the OK from Simon that I can hang onto these instances. I talked to Simon about this and he doesn't see it changing in near term. So I will look at fixing the problem as stated in the title of this bug - passing services into the component constructors. I'll do this as I go about refactoring various components. For most of these services, they are local js service implementations instantiated by the glue code, so it should be safe to hand them around. When we hit upon the case where a component needs a remote service, we'll still need the registry, or else some proxying wrapper (which I think would, in effect, be reimplementing the proxying done by the registry, which would seem wrong). I'll take this bug for now and make sure that the services passed by the glue code are handed in vs. requiring use of the registry. I think that some components will still need the registry (for walking the extension points, etc.) (In reply to comment #0) (As an aside: do we really want to use those Java-isms like > IStatusManager?) no, I did that when I was literally working from the old EAS list. I think we want qualified names anyway. So something like org.eclipse.orion.statusManager ?? while working on bug 337647 I fixed this for the editorContainer. It has no knowledge of the service registry or any services for that matter. It is passed a "statusReporter" which is a proxy for the getService.then pattern. We still have this issue for other components like: - navigator - favorites - outliner I don't think it makes sense to do a lot more here until we settle on how/where the services are initialized. Marking this dependent on bug 334197 moving to M8, awaiting blocking bug. Here is an example of code we have today which is really hard to write and understand because we don't pass the services themselves (from gitCommands.js):
...
exports.getDefaultSshOptions(serviceRegistry).then(function(options){
var func = arguments.callee;
serviceRegistry.getService("orion.git.provider").then(function(gitService) {
serviceRegistry.getService("orion.page.message").then(function(progressService) {
var deferred = gitService.doFetch(path, null, options.gitSshUsername, options.gitSshPassword, options.knownHosts, options.gitPrivateKey, options.gitPassphrase);
progressService.showWhile(deferred, "Fetching remote: " + path).then(
...
Not for 0.2 unfortunately. Since this bug was written, the service access has been changed to be synchronous. So I don't see any technical reason why we couldn't fix this. One more point. I noticed while doing some work on the "related links" implementation that we have assumptions in the code about a service being a real object vs. a service registry proxy.
For example we have code like:
var returnValue = commandService.doSomething()
That works fine if the commandService was instantiated by the glue code and passed in to someone. But if a different client said:
serviceRegistry.getService("orion.page.command") and passed that in to the component, then the component has to do this:
commandService.doSomething().then(function(returnValue) {...});
Or we end up with the pattern
var returnValue = commandService.doSomething();
if (returnValue.then) {
returnValue.then(function(realReturnValue) {...});
} else {
returnValue ... ...
}
I would prefer we not end up with this pattern. Either we always get the proxied service, or we always get the real instance. Note some interfaces will still return a promise even if the service is a local object because something asynchronous is happening.
We really should _always_ be using the service registry (and so a service proxy that always returns a promise) and should never have to resort to checking for "then".
Now with that said ... if we have a call that returns either a promise or regular value you should use dojo.when instead of that returnValue pattern you have below.
e.g.
dojo.when(returnValue, function(realReturnValue) {...});
I've seen a few different bugs today that involve one service that didn't get an expected service passed to it in glue code. Bug 371015 and bug 372158 cause certain operations to fail when the right services are not configured. Bug 372272, is not serious, but could cause a problem down the line, in the same way. When the glue code is not hooked up, we don't notice until something fails. So...if we want to pass in the services we need to assert they are all there up front. Another ramification of passing in services vs. getting them from the registry is asynch vs. synch behavior. For example, if someone passes me a commandService that they created, I'll get synchronous rendering. If they pass me one retrieved from the registry, I'll get asynchronous rendering. When we fix this, search on this bug number in the source code. I know of two places in globalCommands where we are instantiating services that we expect to already be configured (favorites and content types). The basic issue is that all pages use globalCommands. globalCommands expects certain services to be available, but this changes over time and our current architecture requires going back and touching every single page's glue code in order to make sure the right services are there. (In reply to comment #13) > The basic issue is that all pages use globalCommands. > globalCommands expects certain services to be available, but this changes over > time and our current architecture requires going back and touching every single > page's glue code in order to make sure the right services are there. I thought of a more generic way to state the problem as I see it. Using requirejs has solved our dependency problems for what js needs to be loaded. So a page no longer has to know that the banner needs X dialog which needs Y part of dijit which might need Z part if the user clicks on it. But we don't have this same dependency resolution for services. The page *does* need to know that the banner needs service X which might open a dialog that needs service Y which might need Z if the user clicks on it. The "pass the service registry" approach says that everyone just grabs what they need as they need it. Except that if no one (the bootstrap code) ever instantiated the service at all, grabbing it on the fly won't work. We literally have places where we try to grab the service, and if it's not there, we instantiate and register the service on the fly. The "pass in the specific instantiated services" approach says you know in advance, and it becomes safer for a component to assume that the service is actually there because someone passed it in. I like this from a consistency/predictability point of view, but it's very cumbersome and not realistic. Because it means that if I add a feature to Z that needs a new service, that knowledge has to ripple up to all the page code that includes Z. Something like like requirejs that said "here are the services I need" in the components, etc. seems ideal. The components have to say what they need, and these dependencies are resolved to ensure the right services are initialized and available. While working on a different issue, I found another place where our code assumes a service is in-page/synchronous. navigatorRenderer.js, line 115 has var contentType = this.contentTypeService.getFileContentType(item); This is only working because the content type service is passed in by the page code vs. getting accessed from the registry. <rant> I am currently working on bug 390356. It is a not-trivial refactoring of commands so that consumers who don't need a command registry don't have to use it, but can still generate command buttons, etc. That part of the work is complete. Then we had the thought that command service is not really a pluggable service anyway, and so it should just be a helper that is used to generate DOM elements. But I've spent over a full day since then, slogging through all the myriad ways we are accessing the service. Places where we pass it in, or else we pass it in but get it from the registry if we don't need it, etc. Even the straightforward updates touch every single page's glue code. It makes doing the right thing completely cumbersome and then it gets tempting to not bother to do the right thing. We really need a story where modules say what services they need and expect them to be there, and some common piece of code sorts out the dependencies and makes them available. Read comment 14. </rant> as an example: when I took commandService out of the service registry and required clients to pass it in, it took this many changes: http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=477cf3c06a42cd86fb12904fbc2cbd58032b524b Some of this was real work and renaming, but most of the files are just following all the references of the dependency on that service. Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see: https://dev.eclipse.org/mhonarc/lists/orion-dev/msg03444.html |
I've seen code like: registry.getService("IStatusManager").then(function(service) {service.doSomething}); and it looks very complicated. This should not be the default pattern. Rather, I would like to see the service object itself passed to the component that needs it. (As an aside: do we really want to use those Java-isms like IStatusManager?) Are we using the above pattern because we think it is needed for asynchronous services, e.g. if a service is implemented by code that runs in an iframe? I don't see how this would be necessary, since the important thing is to use a then() pattern when you need to access return values from service calls, as in e.g.: preferences.get("popupDelay").then(function(delay) { /*do something with delay */ }); I.e. it is not important that the "preferences" service itself is obtained asynchronously every time it is being used, but it is important that preference values are obtained asynchronously. Only if being lazy about getting the service is important should we be using a more complicated dance. But even then it'd be cleaner to pass the result of registry.getService("IStatusManager") rather than passing the serviceRegistry itself.