Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

New options flag to output ES2015 modules #340

Merged
merged 2 commits into from Nov 21, 2019

Conversation

joseprio
Copy link
Contributor

This PR contains a:

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

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
Copy link
Contributor Author

PR for issue #338

@joseprio joseprio changed the title feat: new options flag to output ES2015 modules New options flag to output ES2015 modules Jul 21, 2019
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.

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

@joseprio
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link

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

@alexander-akait
Copy link
Member

@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
Copy link

@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
Copy link
Member

@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
Copy link

@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
Copy link
Member

alexander-akait commented Nov 20, 2019

@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 😄 )

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
Copy link

@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
Copy link
Member

alexander-akait commented Nov 20, 2019

@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
Copy link

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

@joseprio
Copy link
Contributor Author

joseprio commented Nov 20, 2019

@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
Copy link

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

@jpreynat
Copy link

@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
Copy link
Member

@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
Copy link
Contributor Author

joseprio commented Nov 20, 2019 via email

@jpreynat
Copy link

@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
Copy link
Member

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
Copy link

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

@codecov
Copy link

codecov bot commented Nov 21, 2019

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%> (ø) ⬆️

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
Copy link
Member

just testing, work fine with ModuleConcatenationPlugin

@alexander-akait alexander-akait merged commit 9b9cd8d into webpack-contrib:master Nov 21, 2019
@jpreynat
Copy link

Thanks for the release @evilebottnawi

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

Successfully merging this pull request may close these issues.

None yet

3 participants