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

New config module - storeConfigInMeta #1953

Merged
merged 6 commits into from
May 31, 2024

Conversation

BlueCutOfficial
Copy link
Collaborator

@BlueCutOfficial BlueCutOfficial commented May 30, 2024

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 plugin WriteV1Config. The exact content of the file depended on storeConfigInMeta: if the meta was used, then the content of config/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 the config/environment.js is no longer "rewritten". It's just a module that exists in the initial Ember app at app/config/environment.js and it's moved to the root of the rewritten-app during the build like every other folder in app/ (controllers, components, etc...)

Breaking changes:

  • This first iteration breaks the usage of !storeConfigInMeta. The new environment file calls the library config-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 with isTesting macro.

  • The content for 'config-module' is no longer supported. In addition to providing the script that is now provided by config-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 new app/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.

@BlueCutOfficial BlueCutOfficial force-pushed the new-config-module-storeConfigInMeta branch from 52a94f4 to f3ca4fd Compare May 30, 2024 15:04
@BlueCutOfficial BlueCutOfficial force-pushed the new-config-module-storeConfigInMeta branch from f3ca4fd to fd73602 Compare May 30, 2024 17:34
@BlueCutOfficial
Copy link
Collaborator Author

BlueCutOfficial commented May 30, 2024

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 tsconfig.json is configured to compile all packages to CommonJS, and the command pnpm tsc considers only the first tsconfig.json it founds.

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 tsconfig.json in my package folder.

But this feature forces me to use the -b option and slightly modify the .eslintrc.

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)

@ef4
Copy link
Contributor

ef4 commented May 30, 2024

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: packages/router is excluded by the top-level tsconfig and built separately.

@ef4
Copy link
Contributor

ef4 commented May 30, 2024

(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.)

tsconfig.json Show resolved Hide resolved
@mansona
Copy link
Member

mansona commented May 31, 2024

There is a larger project to make the whole top-level build produce both ESM and CJS for publication

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.

@BlueCutOfficial BlueCutOfficial marked this pull request as ready for review May 31, 2024 09:57
@ef4
Copy link
Contributor

ef4 commented May 31, 2024

@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.

@mansona mansona merged commit 9f1e74e into main May 31, 2024
174 checks passed
@mansona mansona deleted the new-config-module-storeConfigInMeta branch May 31, 2024 18:30
@github-actions github-actions bot mentioned this pull request May 31, 2024
@mansona mansona added the enhancement New feature or request label May 31, 2024
@BlueCutOfficial BlueCutOfficial added breaking and removed enhancement New feature or request labels Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants