-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow services to be provided asynchronously #1128
Comments
@bit-shifter / @hobojed: Do you think this proposal could be made to be compatible with CT? |
@andyberry88 / @james-shaw-turner / @ioanalianabalas: I've updated the asynchronous services proposal based on the meeting we had. @bit-shifter / @hobojed: Can you please also confirm whether I captured everything we discussed, and whether you think this be workable from a CaplinTrader perspective? |
I think this should be fast tracked for the next release. The current state is very confusing to the developer, as you have (in a way) three different service registries: the By implementing this, the only reason (delayed readiness) for the existence of the CT's version goes away (apart from backwards compatibility, off course) and we can unify our code to only use one service registry mechanism. |
@andyberry88 / @james-shaw-turner: let's talk about the need for this the next time you're both around. |
I was trying to convert our default aspect to CommonJS. And after doing that the whole app stopped working, but there weren't any errors. Basically it got stuck somewhere in the delayed readiness madness. |
I've updated this issue to account for the impending ES6 problem. |
@adamshone / @andyberry88 / @briandipalma / @janhancic / @james-shaw-turner / @stevesouth : I've updated this issue based on feedback from @bit-shifter & @hobojed, and am now seeking wider feedback before we consider changing how we do services in BRJS. @briandipalma, I can no longer remember what your remaining objections where to the proposal when we last discussed it, but if you still think this is completely the wrong approach can you please create a counter proposal. Or, if you just think this requires minor tweaks, then it would make more sense to comment directly in-line. Thanks everyone! |
Just to clarify, the first comment in this issue is your current up-to-date proposal? |
Yep, that's correct. All pertinent information is in the first comment. |
Might be worth splitting this into two issues:
|
Yes, that may be a good thing to do from an administrative point of view, but since the design is affected by the complete set of use cases, I prefer to deal with all of it in one place from a design point of view. As it happens, this issue is also solving #1335 too. |
OK. I'm interested in the second problem because it affects testing for everything, not just services. Do you have a strawman code example for a unit test that uses this new mechanism? E.g if the class under test is: var logger = require('br/logger');
function Foo() {
logger.info('constructed');
}
module.exports = Foo; And the test is something like: var logger = /* a mock of */ require('br/logger');
var Foo = require('/foo'); /* but I want to inject the mock logger */
var myFoo = new Foo();
verify(logger.info.withArgs('constructed'); This is the basic test situation that the solution must address, we can also add services to make it more complicated if the solution covers this case. |
@adamshone, okay, then I think I misunderstood your point 2 then, as this issue has never been about allowing require paths to be mocked out. @janhancic created a similar and even more broad reaching issue for this (#1149), but this just seems to be more of the same kind of thing that @briandipalma is warning us not do due to the forthcoming ES6 module compatibility problems we'll face. At present, the sub-realms feature of browser-modules is the only way to do this but, you guessed it, this will also be incompatible with ES6 modules! I'm guessing we either ignore ES6 modules, or start advocating explicit dependency injection in preference to using |
Hello, Good discussion. I have a few questions. I'm not too clear on the introduction, the problems that we currently have part. Why is 1 a problem? The interface doesn't have to reflect the fact the implementation is provided async, that seems to be an implementation detail. How does it cause problems? 2 sounds like a bug, can we not just fix it? Neither issue is clear to me so I'm probably missing something. I'd recommend against exporting instances as the default export, in effect they become singletons and you probably want to test the service class, maybe a well known symbol on the default export i.e. module.exports = ConsoleLogService;
module.exports.INSTANCE = Promise.resolve(new ConsoleLogService()); A similar pattern could be applied to remove the need for the double modules for We could consider using a type base registry in preference to a string based one at this juncture. It would allow tooling support to provide typing support once we move to using something like Flow or TS. var ServiceRegistry = require('br/ServiceRegistry');
var ConfigService = require('caplin/services/ConfigService');
var configService = ServiceRegistry.getService(ConfigService); This would allow us to exploit polymorphism in the typed JS dialects. Strict nominal typing might be problematic in popouts (cross realm checks failing) so we might want to have a duck typing fallback if a service type doesn't match. Although I realize the benefit of this might be quite minor and not worth the hassle so we can just continue with the string based approach. I guess that detecting when and where it is safe to convert libraries to use async registry access will be very tricky and would counsel against such changes, especially as I'm doubtful of the value of such conversions. Also debugging Promises when they fail can be annoying, although the dev tools are improving. Given that we might want to question supporting that extension. I like the prefetch approach, it's unclear how the list of async provided services is kept in sync, I guess application developers will have to create and maintain that list? I'm OK with that, the application developer will be the person with the most knowledge of the app specifics. Since you mention The proposal mentions we want to reduce service injection, it doesn't say why we would though, what are the reasons? From what I remember this isn't quite what was proposed during our meeting, that proposal still relied on the module system and would have had the same caching issues the current approach has. From what I can see this approach uses the fact that BR matches on strings like Also non zero argument services don't seem to be addressed? The approach that we discussed during the meeting on these issues is still worth considering, in essence it's pure code and no configuration, the solution we originally had years ago. Application developers could access the services either as they do now or we could recommend a type based approach instead if we prefer it, deprecating the old approach. var ServiceRegistry = require('br/ServiceRegistry');
var ConfigService = require('caplin/services/ConfigService');
var configService = ServiceRegistry.getService(ConfigService);
// OR
var configService = ServiceRegistry.getService('caplin.config-service'); Application developers would register services to the As many applications will use the same bundles of services we could have modules that they import and which could register bundles of services for them. Some obvious bundles are Using code would require no special consideration for non zero args constructor services or async provided services. Although I think taking a class Service<T> {
static createInstance(): Promise<T> {
}
} I realize this might create more work for application developers but it's clear and requires no special knowledge to understand, it's all standard code. Require/import, instantiate, register. They have complete power and flexibility when it comes to all those steps. |
Hi @briandipalma, comments in-line...
It meant you couldn't change your mind later when you discovered an implementation of a service that needs to be asynchronous that didn't previously need to be. I don't think it was a question of the interface itself, since the asynchronicity was in the provisioning of the service, not in the interface itself. @hobojed knows the most about this.
We had a bootstrapping issue because there were two/three ways of creating service, none of which were guaranteed to dove-tail with the others when there were service inter-dependencies for services provisioned in the different ways:
I created an issue specifically for these bootstrapping issues (#1335), though unfortunately there isn't really any more detailed information then what's here. I'd be happy to talk through exactly how this can cause a problem a call tomorrow if that's helpful?
We avoided the singleton problem precisely by having two modules — Perhaps there is a better way, but I don't really like the
I'm not sure I understand your point, but the current string based approach doesn't prevent you having strongly typed services, or the use of Flow or TS. Whereas, not having a string identifier that we can identify on the server using a regular expression would prevent us from recognizing that an alias is being used, and knowing which concrete class to send to the client.
I'd be interested to know more about this. It's not currently clear to me why it's tricky to know if you can safely convert a library. Specifically, if you can do it without changing the API and breaking backwards compatibility then the asynchronicity has been contained, and will not spread any further.
Yep, the app developer has the responsibility here. The important thing here is that when a synchronous require can't be provided, we need to give a clear error message to the developer like: The 'log-service' service could not be provided synchronously; you will need to either prefetch it before your app starts, or change code to require it asynchronously.
Here I was thinking about how CaplinTrader will provide a different implemenation of the
Because it creates bootstrapping issues. If I inject a service (service
Yes, I think you're quite possibly right that things have may have changed since we had the last conference call. The three intervening weeks and the holiday in between may have meant that things came out differently when I finally got the chance to put things down on paper. My poor memory seems to have helped for once!
They are addressed in that all services are now provided by modules that can either return a promisified service instance, or a non-promisified service constructor — we could support non-promisified service instances too, but
I'm going to responding at this point since I think some of my earlier responses affect the arguments you're putting forward. Let's circle back if you still think this is relevant after you've digested this response. |
Will skip discussing issue 1 as @hobojed has talked to both @dchambers and I and explained that he fixed the bug causing it.
From the description of #1335 it seems that the issue is due to the fact that The identifier doesn't have to be The type based registry was a spur of the moment suggestion and I didn't explain it well. In essence we could use polymorphism to flow the type from the ServiceRegistry to the service instance. class ServiceRegistry {
services: Map<any, any>;
getService<T>(requestedService: { new(): T; }): T {
return this.services.get(requestedService);
}
}
class MyService {
myMethod() {}
}
var sr = new ServiceRegistry();
var myService = sr.getService(MyService);
myService.myMethod();
myService.incorrect(); // Property 'incorrect' does not exist on type 'MyService'. Note this only catches the error in TS, Flow has a few bugs around polymorphism. If you use
In the simple proposal you leave the application developer to register which service implementations they want to use thus removing the need to perform string searches.
You can prevent bubbling up the fact that an operation is triggering async code but you can't prevent interleaving of code that previously didn't. Granted this would not be something I'd expect to cause many issues, but if it does cause any they won't be deterministic, so we could struggle to recreate issues clients can. I think problems would be rare though and this is distracting from the issue so I'll widthdraw my concerns. Can we clarify what
means? Does it mean
So the default service adaptor will always be loaded and an application will have to load & register the CaplinTrader SDK one? That's fine.
Would we not recommend injecting services only for non zero arg constructor services? Chains of those should be rare and unless they are async provided should be simple to deal with. It sounds like we need to document where it makes sense to inject service instances. We should clarify if this approach is working due to the current bundling of code scanning for strings matching
I think I understand, for services that require constructor arguments the application developer should create and inject them into the ServiceRegistry? |
HI @briandipalma, I'm going to focus on a single point you are making, since this seems to be the most defining difference between what I proposed and what you are proposing:
I'd forgotten the essential differences that made me think at the time that you should create a counter proposal, and this is it -- performing IoC using code only. The proposal you make about automatically flowing type information when I think this is an interesting idea, but I think it needs to be further developed in a fully worked through counter proposal. In particular, I'd like to see lots of attention paid to the backwards compatibility issues with existing code, and how we would seek to keep boiler plate to a minimum. @adamshone / @bit-shifter / @hobojed / @janhancic / @james-shaw-turner: what do you guys think? |
I agree on the similarties point. I'm currently fleshing out some ideas around the non string based approach. It's mostly synthesizing the ideas in the issue so far and ideas from our current approach into a summary as I start to lose track of how everything works when it's spread out over so many comments. |
Going back to issue 1
Any service must know whether services it depends upon are synch or asynch so it knows when it can continue. Indeed the |
Hi @briandipalma, I was talking to @james-shaw-turner and was reminded about some of the details of the potential problems I was alluding to with programmatic IoC: Boilerplate: Backwards Compatibility Issues: A Possible Solution?: These were issues that led us to the solution we ended up with in the first place, and which continue to be reasons why our non programmatic approach is still the correct one. |
If I understand correctly we boil down to:
|
Regarding issue 1. Which was
I'm sorry about not providing details on the conversation that @hobojed had with @dchambers and me, it keeps everyone else following the issue in the dark. It appears that the bug that was fixed caused side effects such as the ones you describe
Reading that description is seems that this is an issue introduced by working around the bug. The bug was that a It looks like someone worked around this bug by altering the list of service dependencies for |
OK some light (i hope)! That "fix" copes with the situation where you return a list of supposed async dependent services but a dependency is actually synchronous. However there will be a failure if a dependent async service is omitted from the list of dependent delayed readiness services. The bottom line is that a service needs to understand the async nature of its' dependencies. |
And they can't be changed without breaking backwards compatibility. |
This probably fits into this discussion. Even though proper ES6 modules are years away. We will probably start writing ES6 code soon, with the help of a transpiler. We don't have to wait 5 years before we can start doing that. Wouldn't it then make sense to propose a solution that will be future proof? |
So the plan is to start working on ES6 support for BR in the summer of 2020? |
@janhancic, unfortunately, using a tranpiler like Babel doesn't make it possible to move to fine-grained client-driven module loading as it doesn't support |
I was thinking more in the line of transpiling ES6 imports and stuff into CommonJS. So the browser would get the same things as it does now. The only difference would be that devs write in ES6. |
The module loader polyfill doesn't require that your browser supports HTTP/2. It works just fine without it. |
Ah, okay, yes we'll definitely be able to do that, and the current design also allows the sending of untranspiled ES6 to browsers too. What we've been discussing here though is whether it's possible to have what @briandipalma termed as 'pure' ES6 modules, where there is no server component involved in dependency analysis, and everything is client driven. |
Functionally, yes, but with completely unacceptable performance over a latent connection. |
Has support for creating bundles of your source code so that's not an issue either. |
I know. What I'm trying to say is, that if we are going to allow devs to write ES6 code that then we should come up with a solution that works with ES6. It would be weird writing ES6 code (transpiled) but using a feature that would make it impossible to actually write valid ES6. Hope that makes sense :) |
There's nothing pure about that it's just ES6 modules functioning normally. |
@janhancic, I think you've got the wrong end of the stick here. Nobody is suggesting you won't be able to write ES6 code (transpiled or no). The conversation has been about allowing the use of programmatic IoC for registering services, and how that has serious downsides until browsers support ES6 modules and HTTP/2. |
Thanks for the contribution everyone. Doing a bit more summarising ;-)
No, I want us to start writing ES6 code as soon as practically possible. That will involve transpiling and bundling for the near to medium term. As Dom says once there is full web ecosystem support for ES6 and HTTP2 (and we have a strategy to cope with our legacy code) we won't need most of the features that BRJS provides and we can converge to what is "industry standard" at that time. Doms' summary asserts that Brians approach isn't a practical solution, until we have HTTP2/ES6 native support nirvana. Brians' more recent comments assert there are solutions to the problems Dom forsees e.g.
@briandipalma could you explain in more detail what all the moving parts are in your proposed solution. e.g. It may "have support" what is that support how do we use it? We really need to understand the details and be confident how everything will work before we can make a judgement on the best approach. What software exists off the shelf at the moment and what would we have to write? Please open a new issue with your proposal so we can discuss it in isolation (this thread is already very long !). Also give some thought to backwards compatibility and/or the scope of any changes we would need to make to existing code. |
From looking afresh I've just found that there is something called Not sure what the practical upshot of this is yet, but sharing early... |
It just occurred to me that the ES6 utility classes you'd need to write to support programmatic IoC would need to use |
Depends on the browsers that you have to support. It seems realistic to say that within a year we will have Fx/Chrome and Spartan all supporting the ES6 modules (they already support HTTP/2 and large parts of the ES6 spec). IE8 will no longer be supported by MS and IE9 and IE10 will only be supported on older Windows Server versions and Vista. Neither of which are popular desktop OSes. It's conceivable that new and current projects will be looking to support only the latest Chrome/Fx and IE11+/Spartan. We already have a team working on an app that only supports Chrome why force them and other teams to go through build steps and use special server side machinery when they might not need them?
We have code that is still in use after 7 - 8 years which will probably continue to be used for years. We should be planning for a decade not for 5 years at best and depending on what a team has to support 2 or 3 years. We should be thinking about using standards right now not 5 years down the line when the cost to move to standards is much higher.
This issue is around services, we should focus on making services a future friendly feature. There is one major difference between what is currently available and used in CJS code and the programmatic approach and that is that service implementation classes are registered by the application creator and not by automatic means. Every other detail can be exactly the same. |
This is now covered by #1591. |
At present, BRJS doesn't support services that need to be provided asynchronously. The New Registry Proposal page contains a proposal that aims to address this, and which has taken consideration all of the feedback in this page.
The text was updated successfully, but these errors were encountered: