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

Remove ember-css-modules #167

Merged
merged 8 commits into from
Apr 10, 2023
Merged

Remove ember-css-modules #167

merged 8 commits into from
Apr 10, 2023

Conversation

ijlee2
Copy link
Owner

@ijlee2 ijlee2 commented Mar 29, 2023

Background

ember-css-modules, which lets us localize styles, is not fully compatible with Embroider. Since July 2022, @evoactivity and the Ember community have looked into how else we can implement CSS modules.

About a couple of weeks ago, I began working on embroider-css-modules, a set of tools and conventions to help people implement CSS modules. This pull request shows how people can migrate their apps.

Notes

Commits 2 and 3 should be considered as one (1) step. I have them as separate to credit @buschtoens, who figured out how to more precisely define css-loader's mode().

Commit 7 shows three (3) types of optimizations, which I hand-rolled in order to improve readability and show areas where ember-codemod-remove-ember-css-modules could improve on in the future.

@ijlee2 ijlee2 added the enhance: dependency Issue asks for a new or updated dependency label Mar 29, 2023
@ijlee2 ijlee2 force-pushed the remove-ember-css-modules branch 4 times, most recently from 6506c12 to 1e1e1e6 Compare March 30, 2023 15:28
@ijlee2 ijlee2 marked this pull request as ready for review April 10, 2023 15:21
@ijlee2 ijlee2 merged commit a6888ac into main Apr 10, 2023
@ijlee2 ijlee2 deleted the remove-ember-css-modules branch April 10, 2023 15:22
Comment on lines +8 to +13
{
files: '*.css.d.ts',
options: {
quoteProps: 'preserve',
},
},
Copy link
Owner Author

Choose a reason for hiding this comment

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

Comment on lines +46 to +64
module: {
rules: [
{
exclude: /node_modules/,
test: /\.css$/i,
use: [
{
loader: 'postcss-loader',
options: {
sourceMap: !isProduction(),
postcssOptions: {
config: './postcss.config.js',
},
},
},
],
},
],
},
Copy link
Owner Author

Choose a reason for hiding this comment

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

For more options, including how to configure url-loader, see https://github.com/evoactivity/ember-modern-css#webpack-configuration.

Comment on lines +6 to +12
/*
plugins.push(
require('cssnano')({
preset: 'default',
})
);
*/
Copy link
Owner Author

Choose a reason for hiding this comment

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

@buschtoens found that cssnano may not be needed anymore, because Embroider handles minifying CSS in production.

https://github.com/embroider-build/embroider/blob/v1.8.3/packages/webpack/src/ember-webpack.ts#L332-L340

Comment on lines +27 to +29
export default interface Registry
extends EmberContainerQueryRegistry,
EmbroiderCssModulesRegistry {
Copy link
Owner Author

@ijlee2 ijlee2 Apr 10, 2023

Choose a reason for hiding this comment

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

Comment on lines +29 to +40
mode: (resourcePath) => {
// The host app and active child addons are moved into a common
// stable temp dir (`options.workspaceDir`), before the `css-loader`
// processes them.
//
// We want to enable local mode only for our own host app. All other
// addons should be loaded in global mode.
const hostAppWorkspaceDir = `${options.workspaceDir}/${app.name}`;
const isHostAppPath = resourcePath.includes(hostAppWorkspaceDir);

return isHostAppPath ? 'local' : 'global';
},
Copy link
Owner Author

Choose a reason for hiding this comment

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

I found a couple of cases where mode returns the incorrect value. Please see #188 for the bug fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance: dependency Issue asks for a new or updated dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants