fix: improve esMododule exports#1084
Conversation
alexander-akait
left a comment
There was a problem hiding this comment.
Let's use namedExport, like we have in css-loader
d14131a to
ae3f41f
Compare
ae3f41f to
023d85c
Compare
|
@alexander-akait - I added it to the |
src/loader.js
Outdated
| const exportDefaultString = `export default ${JSON.stringify( | ||
| locals | ||
| )}`; |
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
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?
| /* harmony export */ cnA: () => (/* binding */ _1), | ||
| /* harmony export */ cnB: () => (/* binding */ _2) | ||
| /* harmony export */ cnB: () => (/* binding */ _2), | ||
| /* harmony export */ "default": () => (__WEBPACK_DEFAULT_EXPORT__) |
There was a problem hiding this comment.
We still need an option for this, otherwise we generate extra content, it is bad for a size
|
@jantimon Just want to clarify, do you want to use and default and named export together? |
|
yes for backwards compatibility - right now nextjs allows both nextjs uses mini-css-extract-plugin but is explicitly setting unfortunately so my idea was to switch to esModule but also allow named and default exports just like mini-extract-plugin has also a (currently broken) test on master which mixes default and named imports with esModules: |
|
@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 |
|
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 |
|
@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 |
| 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. |
There was a problem hiding this comment.
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' }).
|
@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
} |
|
@jantimon make sense, but I think better to make it in css-loader |
This PR contains a:
Motivation / Use-Case
With
esModule: falsethe mini-css-extract-plugin generates JS modules like this:This allows the following import:
With
esModule: truethe mini-css-extract-plugin generates JS modules like this:As there is no default export, the following import will not work:
Interestingly the test case
es-named-exporttests for it:https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/es-named-export/index.js
However when tested manually, the test case fails:
This PR adds the
defaultExportoption to the mini-css-extract-plugin which will allow the user to write the import exactly as in the es-named-exports test caseBreaking 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)