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 a config helper #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nvdk
Copy link

@nvdk nvdk commented Sep 22, 2022

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.

extracted from https://github.com/nvdk/ember-config-helper
provides a convenience helper to acces the application config from within the
template. This is useful for template only components mostly.
@bertdeblock
Copy link
Contributor

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
Copy link
Author

nvdk commented Sep 23, 2022 via email

@bertdeblock
Copy link
Contributor

bertdeblock commented Sep 23, 2022

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.

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

Successfully merging this pull request may close these issues.

2 participants