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

Stop using ember-get-config #364

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Conversation

nlepage
Copy link
Contributor

@nlepage nlepage commented Jul 26, 2023

Problem

When running tests from the /tests URL after starting a development server with ember serve, the configuration returned by ember-get-config is stuck to development environment, whereas the configuration returned by the service's owner is from test environment.

This is specific to ember serve, it is not the case when using ember test --server, however using the /tests URL on the development server seems to be a fairly common usage, at least amongst the developers at https://github.com/1024pix.

ember-get-config isn't necessary, configuration can be retrieved via the notification service's owner.

Also quoted from ember-get-config's documentation :

Gaining access to an app's config file from an addon can be challenging. If possible, you should always get it through the container.

Solution

Stop using ember-get-config.

Thanks for your work 🙂

ember-get-config isn't necessary, configuration can be retrieved via the notification service's owner.

When running tests from the /tests URL after starting a development server with `ember serve`, the configuration returned by ember-get-config is stuck to development environment, whereas the configuration returned by the service's owner is from test environment.
This is specific to `ember serve`, it is not the case when using `ember test --server`.
@netlify
Copy link

netlify bot commented Jul 26, 2023

Deploy Preview for ember-cli-notifications ready!

Name Link
🔨 Latest commit 257c681
🔍 Latest deploy log https://app.netlify.com/sites/ember-cli-notifications/deploys/64c10f99c9f600000834526e
😎 Deploy Preview https://deploy-preview-364--ember-cli-notifications.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nlepage
Copy link
Contributor Author

nlepage commented Jul 26, 2023

I found out about #360

We are running on @embroider/compat v3.2.0, and still having the same problem, so it seems that embroider-build/embroider#1412 doesn't fix it...

Copy link
Owner

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue would have gone away if we just updated ember-get-config too 👍 but I'm going to merge this since it's nice to not have an extra dep if we don't need it 🎉

@mansona mansona merged commit 97cd416 into mansona:main Nov 24, 2023
@francois2metz francois2metz deleted the remove-ember-get-config branch November 24, 2023 17:19
This was referenced Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants