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

Design: Replacement Service Registry #1366

Closed
dchambers opened this issue Apr 30, 2015 · 15 comments
Closed

Design: Replacement Service Registry #1366

dchambers opened this issue Apr 30, 2015 · 15 comments

Comments

@dchambers
Copy link
Contributor

This issue is where discussion of the Replacement Service Registry plan should occur.

@dchambers
Copy link
Contributor Author

@adamshone / @andyberry88 / @bit-shifter / @briandipalma / @hobojed / @james-shaw-turner / @janhancic / @stevesouth / any body else that's interested, the plans for a replacement service registry have been evolving based on your feedback, and an updated proposal is now available that it would be good to further feedback on.

I've created a new issue since the original thread had become too long, and wasn't based on this updated proposal. Please help to fix any remaining issues now as this is an important infrastructure piece that will affect all of you in the future.

@janhancic
Copy link
Contributor

I couldn't figure out from this proposal if the problem of requiring services in the top-level is solved.

Certain services (config service, html service) cannot be required outside of a constructor/method/function as they get run when a module is defined. Does this solve the problem?

@adamshone
Copy link
Contributor

I started trying to form an opinion on the new proposal but quickly realised I don't have a good enough understanding of the current implementation to comment on the changes. I've asked @james-shaw-turner if we can talk it through this afternoon.

@dchambers
Copy link
Contributor Author

@janhancic, yes, this problem is solved by moving service requires into the constructor, and using dependentServices to specify the services that should be loaded before the service is constructed.

@dchambers
Copy link
Contributor Author

@adamshone, what, as part of the Architecture meeting at 3pm? Yes, good idea if it doesn't already have an agenda.

@janhancic
Copy link
Contributor

So it's not solved then. We always require our dependencies up front in our code (unless it would be a circular dependency otherwise). This would mean we would have to change a lot of code. A lot.

Also, what does require('service!X') do now? Will it proxy to the new registry? Will the result be cached? Does that mean we still have to use subrelams to clear services down and construct them anew for each test?

@dchambers
Copy link
Contributor Author

So it's not solved then. We always require our dependencies up front in our code (unless it would be a circular dependency otherwise). This would mean we would have to change a lot of code. A lot.

Oh, sorry, I may have confused you, I was specifically talking about services that themselves depend on other services. For normal code you are free to require stuff up top since the app is expected to fetch any dependent services beforehand (via registry.fetch() or ServiceRegistry.initialize()).

Also, what does require('service!X') do now? Will it proxy to the new registry? Will the result be cached? Does that mean we still have to use subrelams to clear services down and construct them anew for each test?

It will continue to proxy to a registry as it does now — while we could theoretically leave it proxying to the old registry (which itself will be updated to proxy to the new registry) it will almost certainly be updated to proxy directly to the new registry.

This issue is making no effort to change how sub-realms works, though we are starting to think about doing things differently ahead of adding ES6 transpilation support (see the half-baked #1303).

@dchambers
Copy link
Contributor Author

Having spoken yesterday with @adamshone, @andyberry88, @james-shaw-turner & @janhancic it was noted that this proposal isn't quite right as it currently stands since, at present, classes are sometimes able to retrieve services without any problems, yet at other times this may fail.

We could fix this by insisting that classes can't request service references at define-time, which can be achieved by making the following changes:

  • Make registry.require() throw an exception if the service doesn't currently exist, rather than 'helpfully' creating it on-demand.
  • Take advantage of the globalization block that most apps will still have, since in tandem with the above change, this will guarantee failure if any classes attempt to request service references at define-time.
  • Leave the service bootstrapping mechanism as currently specified, since this will now always work correctly provided that:
    1. Each service explicitly defines the services it depends on.
    2. Services don't construct any objects, since this might cause additional services to be retrieved that weren't explicitly specified as known dependencies.

Given that the registry is the thing that constructs services, it would even be possible to fail-fast if a service that is being constructed requires any services that weren't explicitly defined as dependencies, since otherwise services may occasionally be able to get away with this, depending on load order.

@janhancic
Copy link
Contributor

We also discussed the following things, for which I don't think we have answers/solutions for yet:

  • rename registry.require() to registry.get()
  • remove service! from the identifiers if possible
  • how will the services be initialised in tests

One additional thing that I'm not clear on is how's the backward comparability going to be. This questions spring to mind:

  • is require('service!foo') still supported? If yes, can I have it anywhere (in a method and in the file)? If I can have it in the file directly, what will it return given that it will be run "define time" when the service isn't actually initialised yet.
  • are still using subrealms to override a services and then re-requireing the classes that need them so they get a "fresh" instance?

@dchambers
Copy link
Contributor Author

  • rename registry.require() to registry.get()

Yes, I think this is a good idea.

  • remove service! from the identifiers if possible

We could do this, but we'd need a way to differentiate between services and aliases, we'd no longer make it possible for CT to modify service behaviour to help with backwards compatibility, and we'd lose the flexibility of being able to add new things into the registry in the future.

Perhaps you have some ideas about this?

  • how will the services be initialised in tests

Test suites will use aliases.xml and individual tests will use registry.adaptor('service').set().

One additional thing that I'm not clear on is how's the backward comparability going to be. This questions spring to mind:

  • is require('service!foo') still supported? If yes, can I have it anywhere (in a method and in the file)? If I can have it in the file directly, what will it return given that it will be run "define time" when the service isn't actually initialised yet.

Yes, still supported, but won't be able to happen at define-time. Unfortunately some re-jigging may be required here, though I believe that doing this was likely to cause problems before too?

  • are still using subrealms to override a services and then re-requireing the classes that need them so they get a "fresh" instance?

Sub-realms aren't used to override a service, they're used to override a module definition. They will continue to work as before.

@dchambers
Copy link
Contributor Author

@janhancic, the other thing that keeps coming to mind is whether we should not bother dropping support for require('service!the-service') in the first place, since that was based on mis-information, and in truth nobody knows what the module loader API is going to end up looking like, whereas it's quite likely to be inspired by the module loader poly-fills that do currently allow modules to be re-defined.

If we went that route than we'd still recommend developers not to require services at define-time, and in fact we'd guarantee to throw an exception for apps having a globalization block.

@dchambers
Copy link
Contributor Author

I've been thinking about the fact that the design so far was based on the mis-information that ES6 modules can't be replaced once they've been cached, whereas we now know this has not yet been decided (see lukehoban/es6features#75). The fact that the current polyfills do allow this is probably a good enough reason for us to continue using this feature since it's necesarry for both the require('service!the-service) syntax, and since it will allow tests that use sub-realms to be compatible with transpiled ES6 libraries.

The fact the Module Loader API for ES2016 will be informed by the polyfills that people use in the interim means that these features may end up becoming standardized anyway, and if not then we can just continue shimming the Module Loader API in future ES2016 compatible browsers.

Given all of that, a design where we don't try to deprecate the service! notation actually looks much simpler:

  • Keep require() as it is.
  • Allow services to define their dependent services.
  • Allow services to return promises that make the service available.
  • Use registerServiceFactory(), fetchServices() & fetchAllServices() to populate the registry (we would provide a deprecated registerService() method too).
  • Allow services to implement br/Service so that fetchAllServices() can work.
  • Stop lazy loading services, and automatically invoke fetchAllServices() if somebody requests a services that hasn't been registered, but warn the developer that they should be doing this themselves.

BRJS Backwards Compatibility Issues:

  1. Developers would need to ensure that any sevices they have written implement br/Service.
  2. Developers would be encouraged to invoke fetchAllServices() in all of their tests to prevent all the warnings.
  3. Developers would be encouraged to switch tests that currently use registerService() to instead use registerServiceFactory(), where they bind the constructor arguments rather than constructing.

CT backwards compatibility Issues:

  1. As for BRJS, since:
    1. DelayedReadinessService could be made to extend br/Service.
    2. ServiceRegistry.initialize() could be made to proxy to fetchAllServices(), etc.

@briandipalma
Copy link
Contributor

I've been thinking about the fact that the design so far was based on the mis-information that ES6 modules can't be replaced once they've been cached, whereas we now know this has not yet been decided (see lukehoban/es6features#75). The fact that the current polyfills do allow this is probably a good enough reason for us to continue using this feature

What I said was that only the exporting module could modify what it exported. Where has this changed in the loader spec? Don't confuse the fact that you can register modules with the ability to change the bindings that have already been created by all the importing modules. Allowing that sort of power would seem to undermine the static nature of ES6 import/exports.

@dchambers dchambers added this to the 1.2 milestone Jul 24, 2015
@dchambers dchambers changed the title Replacement Service Registry Design: Replacement Service Registry Jul 24, 2015
@dchambers
Copy link
Contributor Author

I've created a README.md for a new micro-library (service-box) that will hopefully be what we use as our service-registry outside of BRJS. We can still add support for service! notation within BRJS if we want.

@dchambers
Copy link
Contributor Author

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

5 participants