Skip to content
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

Add missing delayed readiness service handling to ServiceRegistry #560

Closed
bit-shifter opened this issue Apr 22, 2014 · 6 comments
Closed

Comments

@bit-shifter
Copy link
Contributor

The pre-OSS version of ServiceRegistry catered for delayed readiness services (services that are either async or have a dependency on an async service).

This was previously handled via method #initialiseServices, which doesn't appear to have been ported across during the migration.

@dchambers
Copy link
Contributor

@bit-shifter, did you end up doing this for us? If not, how have you worked around this problem in CaplinTrader?

@soruban
Copy link
Contributor

soruban commented Sep 19, 2014

CT is still using their ServiceRegistry which has the method.

We are having the same issue and anyone using, for instance, Permissioning & SLJS services would need this.

@bit-shifter suggested I patch the BRJS ServiceRegistry with the method, unfortunately the initialiseServices adds dependencies to DelayedReadinessService which is not in BRJS at present time, hence this would not be a viable patch to be pulled back.

For now, I just copied the content of the method into our Bootstrap class, once a way to handle this is added to BR, we will switch over.

@bit-shifter
Copy link
Contributor Author

@dchambers Apologies for the late response on this one..

The method has not been ported into brjs, I don't believe that this is the correct thing to do longer term.

A better implementation to satisfy async service issue would be one that uses promises.

@dchambers
Copy link
Contributor

I've just spoken to the CT team about this, and there are a couple of issues with the current approach:

  1. The service alias is expected to point to a class that has a zero-arg constructor, and when then isn't possible the expectation is that the service will be provided on the client.
  2. Some services can not be initialized synchronously, and we handled this in a really bad way by creating a DelayedReadinessService interface, which has a number of issues with it.

Instead, for BRJS we'd like to do the following:

  1. Require all services to return promises regardless of whether they can be provided synchronously or not.
  2. Expect the service alias to point to a module that returns the service, so any service class can be constructed, with our without arguments as necessary, and with no need to inject stuff in.

At this point, it becomes clear that we actually don't need a ServiceRegistry class at all, since a service is just an an aliased modules that returns a promise.

@andy-berry-dev
Copy link
Member

@dchambers Is this something we should aim to add for 1.0?

@dchambers
Copy link
Contributor

This issue has been superseded by #1128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants