[FEATURE ember-application-engines] Initial extraction of engines from applications#12685
[FEATURE ember-application-engines] Initial extraction of engines from applications#12685rwjblue merged 10 commits intoemberjs:masterfrom
Conversation
There was a problem hiding this comment.
Interacting with this before super() in a constructor is not compatible with ES6 classes, fwiw. If this can be differently factored, I think that would be ideal.
There was a problem hiding this comment.
Good catch, @mixonic. I was able to refactor it such that the engine instance is aware that application might be set instead of base (i.e. if base is not set, then application is checked). if others agree that base is general enough to specify the base class for both engine and application instances, we can consider deprecating the application property at some point.
|
Thanks for your work on this @dgeb Cheers |
|
@stefanpenner thanks for the review. I've just revised as per your suggestions, with the two exceptions:
@rwjblue thoughts? |
There was a problem hiding this comment.
I agree, we should likely remove it. ApplicationInstance is not exposed at all, so we should follow that (or expose it as well).
There was a problem hiding this comment.
I debated exposing ApplicationInstance here as well - maybe we should for consistency and then remove both after the feature flag is removed?
There was a problem hiding this comment.
I don't care which way we go, but we need to be consistent. Either both are exposed, or neither are...
|
The doc strings for |
There was a problem hiding this comment.
Can you add a small description here? This is the header page that shows up for the class in our API docs...
@rwjblue Good catch. I just updated |
Confirm. |
…n to Engine. Initializers and instance initializers are a common concern for applications and engines.
…ncerns for engines. Provide a spare base implementation of `buildRegistry` for `Engine`, which is extended by `Application`. Also, allow for a custom `Resolver` property on `Engine` (and by extension, `Application`).
…tialization for engine instances. Move registry and container creation from `ApplicationInstance` to `EngineInstance`.
Only expose `Ember.Engine` when feature flag is enabled.
Replicate appropriate tests from applications. We don’t want to yet move tests while engines are feature flagged and in flux.
Replicate appropriate tests from application instances. We don’t want to yet move tests while engines are feature flagged and in flux.
…mber.ApplicationInstance. Expose Ember.EngineInstance and Ember.ApplicationInstance for easy overriding. Need to reanalyze whether to continue exposing them after feature flag is removed.
|
@rwjblue I've added a brief description for Thanks for the review - let me know if I've overlooked any other suggestions. |
[FEATURE ember-application-engines] Initial extraction of engines from applications
5 participants
This initial PR for engines follows the strategy proposed in the Engines RFC.
It introduces a base class,
Engine, which is wholly extracted from (and then extended by)Application. Similarly,EngineInstancehas been wholly extracted from (and then extended by)ApplicationInstance.The
Enginebase class is now responsible for maintaining a registry and initializers.The
EngineInstancebase class is now responsible for maintaining an instance-level registry and container.Ember.Engineis only exposed when the newember-application-enginesfeature flag is enabled.The APIs and behavior of
ApplicationandApplicationInstanceremain identical, as do tests and documentation.Some tests of extracted behavior were duplicated for engines, but the original tests for applications were kept in place. Any test duplication should be revisited before the feature flag is enabled.
The primary purpose of this PR is to allow for further experimentation with engines in an addon. It will also allow behavior to be folded into ember core gradually as experimentation proceeds.