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

Share / Re-import host app's config #176

Open
mydea opened this issue Aug 8, 2016 · 7 comments
Open

Share / Re-import host app's config #176

mydea opened this issue Aug 8, 2016 · 7 comments

Comments

@mydea
Copy link
Contributor

mydea commented Aug 8, 2016

Is there a way to share or re-import the host app's config in an engine?
Concretely, it would be nice to not need to re-declare application config. For example, things like:

moment: {
  outputFormat: 'MMM',
  includeLocales: ['en', 'de', 'de-at', 'es', 'en-ie', 'en-gb', 'en-au', 'en-ca', 'ja', 'ko'],
  includeTimezone: '2010-2020'
}

Need to be declared twice, even though I guess it is common that these things should be the same for the host app and the engine. The same goes for API host configuration etc.

Is there a way to do that? I tried doing import hostConfig from '....' in the engine's config, but that didn't work.

@stefanpenner
Copy link
Collaborator

stefanpenner commented Aug 8, 2016

It seems like we may want to encourage duplicate here rather then dryness. But would love to here counter arguments.

@nathanhammond
Copy link
Contributor

I much prefer duplication in an engine as it is intended to be an isolation primitive.

@trentmwillis
Copy link
Member

We need to think of the Engine as separate (isolated) from the App. Therefore, I like to think of it as the Engine needs a Moment config, and the App has a Moment config. Therefore, if we want to avoid duplication then the App has to give the Moment config to the Engine (instead of the Engine taking the config from the App).

Alternatively, if we think in terms of duplication, then the Engine should have its own config. It may be that the Engine's config is the same as the App's config, but to assume they are is bad and couples you to the implementation of the App (breaks isolation).

Here are some quick diagrams to illustrate what I'm saying:
Passing configs in Engines

@dgeb
Copy link
Member

dgeb commented Aug 8, 2016

Nice diagrams @trentmwillis 💯

I agree with others that we don't want an engine to reach directly into its host app's config (Trent's "bad" illustration of breaking isolation).

On the other hand, the engine itself can only have generic config settings that don't allow for variations in where it's hosted. I believe there is value in allowing a host to provide host-specific config settings to an engine (this is covered by Trent's "good" illustration of passing settings forward).

We still need to define an interface for this "good" facility to share settings. The only current alternative is to share settings via a service dependency, which is not terrible but probably not the best fit for config settings.

@nathanhammond
Copy link
Contributor

/cc @twokul @pzuraq

@pzuraq
Copy link

pzuraq commented Aug 9, 2016

I agree with @dgeb that a service dependency is not ideal for sharing configuration across engines. There are a number of addons that implement configuration this way, and it's a headache. Configuration state can be determined at build time, so we should leverage that.

From the context of addon configuration, apps and parent addons want to be able to override default properties of child addons. Engine behavior might be a little more nuanced, in that engines may either want to inherit default properties they can then override, or have their own properties overriden by the parent. I can see use cases for both styles, and the only API I can think of that would solve both cases would be some sort of priority level for configuration. Having worked with Chef and CSS, where !important and override attributes cause headaches and confusion regularly, I don't think this is a good idea.

So, in order to keep the API simple, I think parents should be able to pass forward properties to their children, and those properties should always take precedence. It's a much simpler API that way, and if we determine that priority levels are needed in the future we can add them without breaking changes.

As for the API, I've been thinking that we could add /config/addons and /config/engines folders, and maintain a separate file for each addon/engine. Files would be named after their corresponding addon/engine, and properties would be merged downward toward the leaf-most nodes, until a set of final configs is created for the addon/engine. For engines, only one config will exist, but for addons a different config can exist per engine (if, for instance, the root app does not specify a value for a given addon and two engines then specify different values).

We would then add the ability for addons/engines/apps to import a special config helper which would inject the proper value when the factories for objects are created in the container. I'm still trying to figure out the best interface for that, but I expect it would look something like:

import config from 'addon-name/config';

export default Ember.Component.extend({
  someValue: config.inject('someValue')
});

The key thing here is we need both the context of the engine/container that the factory is being created in and the context of the addon that it belongs to, in order to get the proper config. The config helper would be created with the context of the addon, and the container it's being instantiated in would provide the context of which engine it's being created in.

Alternatively, we could say that only one config can exist per addon per app. If that were the case, the system becomes much simpler - we just collapse the configurations down to a final JSON file and export it at addon-name/config or similar:

import config from 'addon-name/config';

export default Ember.Component.extend({
  someValue: config.someValue
});

Conflicting configuration values in this case should probably throw an error to let users know. I like this method for its simplicity, but I do think it would be much less flexible. However, if users really needed to have different configs per engine, they could default to the services method that is being used currently, as it works for that use case.

/cc @null-null-null

@nathanhammond
Copy link
Contributor

Responded off thread to @pzuraq, encouraging opening an RFC for Ember CLI. This is definitely a problem that we encounter often and we need to have a great solution for it going forward.

/cc @kratiahuja @zackthehuman

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

No branches or pull requests

6 participants