-
Notifications
You must be signed in to change notification settings - Fork 142
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
New config module - storeConfigInMeta
#1953
Conversation
…or canary-macro-sample-addon scenario in the addon-template
52a94f4
to
f3ca4fd
Compare
…the config <meta> in the document
…moving the update of the config on fixture side
f3ca4fd
to
fd73602
Compare
I'd need feedback regarding how we want to deliver the new library (see this commit). Details 👇I wanted to export an ESModule but the root It was handy for my development work to keep the new package inside the monorepo, so I used the feature project references + composite to have a second But this feature forces me to use the Does the solution look fine or do we want to use a separate repo as the package doesn't properly need the other Embroider packages? (this way there will be no change in these configs) |
I think it's a good idea to ship this new library as real ES modules instead of CJS. It would be OK to special-case this package in the monorepo to be different from the rest. There is already precedent for this: |
(There is a larger project to make the whole top-level build produce both ESM and CJS for publication, but we haven't prioritized it.) |
…while storeConfigInMeta is broken
I think this PR gives us a much better way to get this project done. I had never considered that we could PR the "proper module" implementation of each of the packages one-by-one using this trick but I think this PR paves a way for us. |
@mansona made me realize that my comments above were not clear: I am in favor of this general approach and was suggesting the router package as a good precedent to emulate, for when we want to break out of the top-level build. |
Context
This PR is a step to remove the rewritten app and a first iteration to have a more standard environment config.
Before, stage 2 was writing
config/environment.js
in the rewritten-app using the Broccoli pluginWriteV1Config
. The exact content of the file depended onstoreConfigInMeta
: if the meta was used, then the content ofconfig/environment.js
relied on content for'config-module'
functionality; therefore, the content was the script reading the meta in the document + the potential overrides provided by classic addons (example in ember-cli-fastboot).Now,
WriteV1Config
is completely removed and theconfig/environment.js
is no longer "rewritten". It's just a module that exists in the initial Ember app atapp/config/environment.js
and it's moved to the root of the rewritten-app during the build like every other folder inapp/
(controllers, components, etc...)Breaking changes:
This first iteration breaks the usage of
!storeConfigInMeta
. The new environment file calls the libraryconfig-meta-loader
that reads the config<meta>
in the document (like the Broccoli plugin app-config-from-meta does in classic builds), so this meta must be present. ℹ️!storeConfigInMeta
will be implemented later for the new format, but it implies solving a more tricky issue withisTesting
macro.The content for
'config-module'
is no longer supported. In addition to providing the script that is now provided byconfig-meta-loader
, it could also be used to let classic addons modify this script (see example in ember-cli-fastboot). We won't support this feature anymore because thanks to the newapp/config/environment.js
, developers have full control about what the environment module does, it's no longer something hidden in the build process that your addon needs to temper with.