mansona / ember-get-config

Get `config/environment` from anywhere, even addons!!!
MIT License
65 stars 20 forks source link

add a config helper #52

Open nvdk opened 2 years ago

nvdk commented 2 years ago

As promised at emberfest: the PR :) This is extracted from https://github.com/nvdk/ember-config-helper and provides a convenience helper to access the application config from within the template. This is useful for template only components mostly.

At the moment the helper doesn't use the config import provided by ember-get-config, this can be adjusted if that makes more sense.

bertdeblock commented 2 years ago

M2C, I don't think it's a good idea to include this helper, because:

  1. A class-based helper is container aware, adding this goes against the reason this addon exists, if users want config access in template-only components, they should simply install ember-config-helper directly
  2. The main users of this addon are other addon authors (I hope) who want easy (lazy imo) access to the app's config in module scope, including this helper will lower the threshold for apps to start using this addon, which might mean they also end up doing import config from 'ember-get-config'; instead of import config from 'my-app/config/environment';

Even though this addon now works under Embroider as well, I feel that apps should start pushing config towards addons, instead of addons pulling in the app's config, like you would do with any vanilla package, you import a setup function and you call it with some config. This mental switch might not happen if we keep adding new functionality to this addon. The pull approach also forces app authors to always configure addons via config/environment.js and prevents them from storing addon config anywhere else in their app.

nvdk commented 2 years ago

That's fair criticism, i think your first point could be addressed easily by converting to a helper function and leveraging ember-get-config. The second is more of a philosophical discussion that I don't really have an opinion on.

bertdeblock commented 2 years ago

i think your first point could be addressed easily by converting to a helper function and leveraging ember-get-config.

My first point is mainly about the fact that users should just install ember-config-helper directly if they need config access in template-only components. ember-config-helper uses the right implementation, a class-based / container-aware helper that avoids using ember-get-config. Pulling in the config helper and making it a functional one, only to use ember-get-config feels backwards imo.