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

fix: improve esMododule exports #1084

Merged

Conversation

jantimon
Copy link
Contributor

@jantimon jantimon commented Feb 28, 2024

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

With esModule: false the mini-css-extract-plugin generates JS modules like this:

module.exports = {
  foo: "src-styles-foo",
  bar: "src-styles-bar",
};

This allows the following import:

import * as styles from "./styles.css";
import styles from "./styles.css";
import { foo } from "./styles.css";

With esModule: true the mini-css-extract-plugin generates JS modules like this:

export {
  "foo": "src-styles-foo",
  "bar": "src-styles-bar"
};

As there is no default export, the following import will not work:

import styles from "./styles.css"; // Fails

Interestingly the test case es-named-export tests for it:

https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/es-named-export/index.js

import css, { aClass, bClass, cClass } from "./style.css";

However when tested manually, the test case fails:

export 'default' (imported as 'css') was not found in './style.css'

This PR adds the defaultExport option to the mini-css-extract-plugin which will allow the user to write the import exactly as in the es-named-exports test case

Breaking Changes

This feature is introduces behind an option.
Therefore this is a non-breaking change.
However changing the named export would probably also be not a breaking change)

need to check behaviour webpack/webpack#18271 (comment)

Copy link

linux-foundation-easycla bot commented Feb 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's use namedExport, like we have in css-loader

@jantimon jantimon changed the title feat: add defaultExport option fix: improve esMododule exports Feb 29, 2024
@jantimon jantimon force-pushed the hybrid-es-module-export branch from d14131a to ae3f41f Compare February 29, 2024 08:27
@jantimon jantimon force-pushed the hybrid-es-module-export branch from ae3f41f to 023d85c Compare February 29, 2024 08:28
@jantimon
Copy link
Contributor Author

@alexander-akait - I added it to the namedExport case - is it okay like this?

src/loader.js Outdated
Comment on lines 274 to 276
const exportDefaultString = `export default ${JSON.stringify(
locals
)}`;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const exportDefaultString = `export default ${JSON.stringify(
locals
)}`;
const exportDefaultString = `export default { ${identifiers
.map(([id, key]) => `${JSON.stringify(key)}: ${id}`)
.join(", ")} }`;

That would be a bit more efficient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done:

--- a/test/cases/es-named-export-as-is/expected/main.js
+++ b/test/cases/es-named-export-as-is/expected/main.js
@@ -17,7 +17,7 @@ var _1 = "Xh041yLR4iCP4RGjge50";
 var _2 = "NMuRsxoDwvW8BhSXhFAY";
 var _3 = "ayWIv09rPsAqE2JznIsI";
 
-/* harmony default export */ const __WEBPACK_DEFAULT_EXPORT__ = ({"a-class":"Xh041yLR4iCP4RGjge50","b__class":"NMuRsxoDwvW8BhSXhFAY","cClass":"ayWIv09rPsAqE2JznIsI"});
+/* harmony default export */ const __WEBPACK_DEFAULT_EXPORT__ = ({ "a-class": _1, "b__class": _2, "cClass": _3 });

could this somehow prevent the minifier from optimizing?

@jantimon jantimon requested a review from sokra March 1, 2024 12:04
@@ -8,12 +8,14 @@
__webpack_require__.r(__webpack_exports__);
/* harmony export */ __webpack_require__.d(__webpack_exports__, {
/* harmony export */ cnA: () => (/* binding */ _1),
/* harmony export */ cnB: () => (/* binding */ _2)
/* harmony export */ cnB: () => (/* binding */ _2),
/* harmony export */ "default": () => (__WEBPACK_DEFAULT_EXPORT__)
Copy link
Member

Choose a reason for hiding this comment

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

We still need an option for this, otherwise we generate extra content, it is bad for a size

Copy link
Member

Choose a reason for hiding this comment

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

/cc @jantimon

@alexander-akait
Copy link
Member

@jantimon Just want to clarify, do you want to use and default and named export together?

@jantimon
Copy link
Contributor Author

jantimon commented Mar 5, 2024

yes for backwards compatibility - right now nextjs allows both

nextjs uses mini-css-extract-plugin but is explicitly setting esModule: false

unfortunately esModule: false has the drawback that it prevents webpacks esmodule optimizations

so my idea was to switch to esModule but also allow named and default exports just like esModule: false

mini-extract-plugin has also a (currently broken) test on master which mixes default and named imports with esModules:

https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/es-named-export/index.js

@alexander-akait
Copy link
Member

@jantimon I see, we need to verify webpack (experimental.css) work the same (or provide an option to do it) and align with this plugin, I will investigate it tomorrow

@alexander-akait
Copy link
Member

Sorry for delay, I done the same for built-in CSS support - webpack/webpack#18271 but want some dicussion with trspack team to align behaviour, then it will be merged I will do the same here

@alexander-akait
Copy link
Member

alexander-akait commented Apr 2, 2024

@jantimon I put it under the option, because we still recommend to use only a named export to be better compatibility with future https://web.dev/articles/css-module-scripts

@alexander-akait alexander-akait merged commit 74ae781 into webpack-contrib:master Apr 2, 2024
33 checks passed
By default, `mini-css-extract-plugin` generates JS modules based on the `esModule` and `namedExport` options in `css-loader`.
Using the `esModule` and `namedExport` options will allow you to better optimize your code.
If you set `esModule: true` and `namedExport: true` for `css-loader` `mini-css-extract-plugin` will generate **only** a named export.
Our official recommendation is to use only named export for better future compatibility.
Copy link

Choose a reason for hiding this comment

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

Just for curious, what does "better future compatibility" refer to here? If it's about native CSS modules (https://web.dev/articles/css-module-scripts), it should be distinguishable because of the need for the import-attributes/import-assertions syntax (assert { type: 'css' }).

@jantimon jantimon deleted the hybrid-es-module-export branch April 3, 2024 06:59
@jantimon
Copy link
Contributor Author

jantimon commented Apr 3, 2024

@alexander-akait thanks for merging 🎉

Initially I added the default export also behind a feature flag so it's totally fine for me that you brought that back..

However I have one question - should the following css module code cause a warning for better future compatibility?

example.module.css

.default {
  color: red
} 

@alexander-akait
Copy link
Member

@jantimon make sense, but I think better to make it in css-loader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants