fix(ember): Fix backwards compatibility with Embroider changes #3230
Conversation
Switching from ember-get-config to macros introduced a regression for the classic build system, since Embroider injects runtime config differently depending on whether the app is using embroider for it's build system or not. This will grab the config and add it in the included hook when building the addon and including it in an app if embroider is not detected.
size-limit report
|
…-backwards-compatibility
packages/ember/index.js
Outdated
| @@ -16,6 +16,21 @@ module.exports = { | |||
| }, | |||
| }, | |||
|
|
|||
| getAddonConfig(app) { | |||
| const config = require(app.options.configPath)(app.env); | |||
There was a problem hiding this comment.
Is it always given that configPath will be at least defined? If not, then we should either wrap it in try/catch or if (!app.options || app.options.configPath)
There was a problem hiding this comment.
config path should be always defined, since it's pulling the conventional config/environment.js which should always exist, but I think we can wrap this in try/catch anyway in case someone has an edge case 👍
| }, | ||
|
|
||
| included() { | ||
| this._super.included.apply(this, arguments); |
There was a problem hiding this comment.
This super call should be fine.
|
@k-fish I just tried this out and it does not appear to work for me. I'm using an environment variable to switch between embroider and non-embroider builds but the condition in this MR assumes that as soon as the dependency exists it will always use embroider. I've tried to hardcode the condition temporarily, but even then it does not appear to work since |
|
It also breaks for me - I do not use embroider to build the app but have embroider/macros installed and use it for some stuff, so it tries to use the new config but fails at build time with a non-helpful message ( |
As mentioned in #3230, this is still causing problems for build systems. Pulled out the logic to put config into an embroider macro to the 'config' hook for the addon, which gets rid of the conditional logic as well as pulling configPath which could be undefined. Tested on both sides of using Embroider and the existing build system and the macros appear to be populated now, so this should work.
|
@Turbo87 right, I hadn't considered the use case of flipping between systems, thanks 👍. @mydea definitely should not be breaking still, the semver incompatibility isn't intentional, was hoping to roll forward the fix instead. Just put up another fix that should work on both sides more cleanly now (using the See #3246 |
* fix(ember): Fixing fetching config during build step As mentioned in #3230, this is still causing problems for build systems. Pulled out the logic to put config into an embroider macro to the 'config' hook for the addon, which gets rid of the conditional logic as well as pulling configPath which could be undefined. Tested on both sides of using Embroider and the existing build system and the macros appear to be populated now, so this should work.
* fix(ember): Fixing fetching config during build step As mentioned in #3230, this is still causing problems for build systems. Pulled out the logic to put config into an embroider macro to the 'config' hook for the addon, which gets rid of the conditional logic as well as pulling configPath which could be undefined. Tested on both sides of using Embroider and the existing build system and the macros appear to be populated now, so this should work.
Summary
Switching from ember-get-config to macros introduced a regression (#3181) for the classic build system, since Embroider injects runtime config differently depending on whether the app is using embroider for it's build system or not. This will grab the config and make it available to embroider/macros if embroider/core is not detected.