webpack-contrib / file-loader

File Loader
MIT License
1.86k stars 257 forks source link

New options flag to output ES2015 modules #340

Closed joseprio closed 4 years ago

joseprio commented 5 years ago

This PR contains a:

Motivation / Use-Case

Using the old CommonJS module syntax breaks some optimizations that Webpack does in newer versions, like tree shaking and module concatenation. This flags allows the output to be ES2015 modules if enabled.

Breaking Changes

Default is false, so it shouldn't break anything unless used.

Additional Info

joseprio commented 5 years ago

PR for issue #338

joseprio commented 5 years ago

Good job, we should improve supporting esmodules for css-loader, maybe for html-loader, just try to add tests for css-loader and you can see problem, anyway feel free to send a PR to css-loader

html-loader already has support for it; the option is exportAsEs6Default. Will check css-loader.

joseprio commented 5 years ago

Just checked css-loader; it would be a more complex process than mini-css-extract-plugin, html-loader and file-loader, but definitely doable. However, I don't think it's a valuable feature to add. My understanding is that css-loader output is used as the input for plugins like postcss, so the final output won't be a JS module, but a CSS text file; so, the advantages of using ES6 modules in Webpack are not relevant.

alexander-akait commented 5 years ago

@joseprio no, look here https://github.com/webpack-contrib/css-loader/blob/master/src/runtime/getUrl.js#L1, now it is not string, it is object with default property

jpreynat commented 4 years ago

Hi @evilebottnawi, Any chance we could have this PR merged soon?

alexander-akait commented 4 years ago

@jpreynat Why do you need this? In webpack@5 we have asset built-in modules (it is like file-loader and url-loader), so package will be deprecated after stable release webpack@5

jpreynat commented 4 years ago

@evilebottnawi Thanks for the info, I didn't see this new experiment on webpack 5. However, it seems that the asset plugin in webpack 5 doesn't generate ES modules either.

Also, it seems to lack configuration. For instance, with file-loader we were able to omit the generation of the file when it wasn't needed, and specify a specific directory for different types of modules. For instance, we could test .(png|jpg) and emit them as images/[hash].[ext], while testing for .css and emit them as styles/[hash].[ext].

That's a lot of things missing for us with the current state of the asset experiments flag.

alexander-akait commented 4 years ago

@jpreynat

it seems that the asset plugin in webpack 5 doesn't generate ES modules either.

// TODO: (hiroppy) use ESM will be done in near future

For instance, with file-loader we were able to omit the generation of the file when it wasn't needed

Can you clarify?

specify a specific directory for different types of modules For instance, we could test .(png|jpg) and emit them as images/[hash].[ext], while testing for .css and emit them as styles/[hash].[ext].

You can use test/include/exclude for filtering.

Look in assetModuleFilename option. It can be function so you can create any custom logic.

That's a lot of things missing for us with the current state of the asset experiments flag.

Which?

The file-loader will be deprecated after webpack@5 was released.

jpreynat commented 4 years ago

@evilebottnawi Thanks for the info.

Great if switching to ESM can be done quickly. I will also have a look at the assetModuleFilename option. I thought that it could only be a string, but looks like a function should do the job.

Finally, we were using the emitFile: false option for file-loader when bundling for Node. Can we easily replicate this behavior with the asset type in modules rules?

alexander-akait commented 4 years ago

@jpreynat

Great if switching to ESM can be done quickly.

Yes, we will do it, but i can't understand why you need modules here? It is always will be interpreted as default (you can't use named import for assets :smile: )

Finally, we were using the emitFile: false option for file-loader when bundling for Node. Can we easily replicate this behavior with the asset type in modules rules?

In Todo, but why don't use null-loader for this when you build node project?

jpreynat commented 4 years ago

@evilebottnawi For insights on our use-case, we are currently working on reducing the initial load time of our application, and since our app has many dependencies, assets, etc... it turns out that lots of initial time is spent in webpack_requires.

Using ES modules should allow webpack to further concatenate our bundle by using module concatenation for our assets too.

alexander-akait commented 4 years ago

@jpreynat

Using ES modules should allow webpack to further concatenate our bundle by using module concatenation for our assets too.

Please clarify? Do you mean include url to asset without extra module, right?

jpreynat commented 4 years ago

@evilebottnawi Yes, basically we want to prevent having to require a new module for each of our assets.

joseprio commented 4 years ago

@jpreynat This is the exact purpose for this pull request, as described in the 'motivation' part; we currently patch webpack in order to take advantage of that optimization, and it definitely helps. I'm sure other projects could take advantage of it, and migrating to version 5 (once it actually can output ESM) may not be an option for everybody

jpreynat commented 4 years ago

@joseprio Thanks for confirming that. This is why I asked for an update on the PR in the first place πŸ‘

jpreynat commented 4 years ago

@joseprio Could you share any insights on how you managed to tweak webpack in order to achieve this on your side? Was it simply to work with a fork of file-loader or did you make other/further optimizations?

alexander-akait commented 4 years ago

@jpreynat okay, let's merge it and released, also i will add tests for this cases for asset module to ensure all works fine in webpack@5

Can you answer about emitFile?

joseprio commented 4 years ago

We're just using patch-package; not ideal from a maintenance point of view, but does the trick.

On Wed, 20 Nov 2019 at 14:24, Johan Preynat notifications@github.com wrote:

@joseprio https://github.com/joseprio Could you share any insights on how you managed to tweak webpack in order to achieve this on your side? Was it simply to work with a fork of file-loader or did you make other/further optimizations?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/webpack-contrib/file-loader/pull/340?email_source=notifications&email_token=AAMTJU2I6MWJN2H3T3P6KQ3QUVCBPA5CNFSM4IFSYDSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEESEJWY#issuecomment-556025051, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMTJU76NNS64V27QVPKK4DQUVCBPANCNFSM4IFSYDSA .

jpreynat commented 4 years ago

@evilebottnawi Great news, thanks. It's ok for us to use the new asset experiment, but we should make sure that this is working as expected with module concatenation.

Regarding the emitFile, we simply turn it to false when compiling for Node to prevent exporting our assets in our serverless functions bundle. If the same could be achieved with the asset type, it would be awesome. Something like if null is returned from the assetModuleFilename function, then the file is not emitted, would do the job.

@joseprio I didn't know this package. This seems indeed a bit hacky, but sometimes we've got no choice πŸ˜‰ Thanks for sharing the tip πŸ‘

alexander-akait commented 4 years ago

Something like if null is returned from the assetModuleFilename function, then the file is not emitted, would do the job.

Maybe not bad idea

jpreynat commented 4 years ago

Just a suggestion, but that would allow to fine-grain process the assets.

codecov[bot] commented 4 years ago

Codecov Report

Merging #340 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #340   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          28     29    +1     
  Branches       11     13    +2     
=====================================
+ Hits           28     29    +1
Impacted Files Coverage Ξ”
src/index.js 100% <100%> (ΓΈ) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update ba0fd4c...16519c9. Read the comment docs.

alexander-akait commented 4 years ago

just testing, work fine with ModuleConcatenationPlugin

jpreynat commented 4 years ago

Thanks for the release @evilebottnawi