feat: emitFile option

[ferdinando-ferreira]

This PR contains a:

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

Motivation / Use-Case

Add emit option (default: true).

• If true, emits a file (writes a file to the filesystem).
• If false, the plugin will extract the CSS but will not emit the file.

The use case for this this option is the same of the already existing on file-loader (see https://github.com/webpack-contrib/file-loader#emitFile): to disable the actual emission of the file, useful for server-side packages

Breaking Changes

• n/a

Additional Info

In order to test an use case demonstration was prepared.

git clone https://github.com/ferdinando-ferreira/mini-css-extract-plugin --single-branch mini-css-extract-plugin
cd mini-css-extract-plugin
npm install
cd usecase
npm install
npm run build

It generates 3 compilations for the same source, with different parameters

dist/current-plugin: uses the current plugin as is. Generates the dist/ssr/index.css, which is what the creation of the new option is trying to suppress, as the file is useless for SSR

 - nonssr:
     - 1042K  index.css
     -  249K   index.js
 - ssr:
     - 1042K  index.css
     - 1666K  index.js

dist/intended-behaviour: uses the proposed new option emit. Does not generate the dist/ssr/index.css, which is exactly what the new option is set to achieve

 - nonssr:
     - 1042K  index.css
     - 249K   index.js
 - ssr:
     - 1666K  index.js

dist/proposed-alternative-1: uses the alternative proposed by @alexander-akait in #713 (comment) ("just don't use this plugin for SSR") which, while not generating the dist/ssr/index.css, it does that by leaving it unextracted from the index.js, which fails to achieve the goal.

 - nonssr:
     - 1042K  index.css
     - 249K   index.js
 - ssr:
     - 2711K  index.js

[alexander-akait]

Why you need this plugin for SSR?

[ferdinando-ferreira]

Why you need this plugin for SSR?

It is explained in the PR. Without the plugin the resulting ssr index.js contains the unextracted css.

[alexander-akait]

Sorry, again, just ignore CSS require/import for SSR

[ferdinando-ferreira]

Sorry, again, just ignore CSS require/import for SSR

That is also explained in the PR provided usecase and also not possible. The css is not "required" or "imported" but extracted from the .vue single file component by vue-loader and passed to this plugin for extraction.

[alexander-akait]

In this case you need to open an issue in vue-loader

[ferdinando-ferreira]

In this case you need to open an issue in vue-loader

Vue loader delegates css extraction (see https://vue-loader.vuejs.org/guide/extract-css.html#webpack-4) to this plugin.

Extracting css is the main and only reason this plugin exists. file-loader, the equivalent for the other file types implements it already.

If there is no technical reason not to: why the resistance to implement it?

[alexander-akait]

Because we don't need this option:

• a lot of developers use SSR and don't need this options (for many years no one issue about it)
• you should not use this plugin for SSR, because it does not make sense
• avoid emitting assets is not safe, we update hashes but do not emit nothing, it is bad for caching

[alexander-akait]

Also implementation is invalid, because we don't need process something in loader, we can just return empty content

[ferdinando-ferreira]

a lot of developers use SSR and don't need this options

Because they would just sink the files using ignore-emit-webpack-plugin

(for many years no one issue about it)

#74, nearing 3 years now and that had other parties interested, not just myself.

you should not use this plugin for SSR, because it does not make sense

It does make sense. If you don't extract the CSS it remains in the .js file. It makes index.js (in this example) a 2.7M files instead of a 1.6M.

avoid emitting assets is not safe, we update hashes but do not emit nothing, it is bad for caching

This is not a technical issue. The new option is optional and does not change the default behavior, be it for cache or otherwise. A warning in the doc saying passing false to this option may be bad for caching (whatever that means) would be enough.

[alexander-akait]

Because they would just sink the files using ignore-emit-webpack-plugin

Can you provide link on docs or examples?

#74, nearing 3 years now and that had other parties interested, not just myself.

This issue from you

It does make sense. If you don't extract the CSS it remains in the .js file. It makes index.js (in this example) a 2.7M files instead of a 1.6M.

But you can just ignore all require/import for CSS

This is not a technical issue. The new option is optional and does not change the default behavior, be it for cache or otherwise. A warning in the doc saying passing false to this option may be bad for caching (whatever that means) would be enough.

We don't want to provide unsafe options

[ferdinando-ferreira]

This issue from you

Even after I closed it other person asked to reopen because of a similar need.

Can you provide link on docs or examples?

Here is the same option on file-loader (https://github.com/webpack-contrib/file-loader#emitFile)

It is often useful to disable this option for server-side packages.

They implemented this in 2016 (webpack-contrib/file-loader@20c8fff), it is useful.

But you can just ignore all require/import for CSS

How?

[alexander-akait]

Can you provide link on docs or examples?

I mean for SSR and CSS, no need to link on file-loader, there other logic

[ferdinando-ferreira]

I mean for SSR and CSS, no need to link on file-loader, there other logic

There is no difference. SSR and CSS (in this plugin) or SSR and a PNG (for file-loader) are the same case. It just wastes space in the javascript or disk

[ferdinando-ferreira]

Why you need this plugin for SSR? (...) you can just ignore all require/import for CSS

You are correct that the option is not really required to be in this plugin, one can just use null-loader

const webpack = require('webpack');
const path = require('path');
const { VueLoaderPlugin } = require('vue-loader');
// const MiniCssExtractPlugin = require('../../../dist/cjs.js');
module.exports = {
  mode: 'development',
  target: 'node',
  output: {
    path: path.join(__dirname, '..', '..', 'dist', 'current-plugin', 'ssr'),
    publicPath: '/assets'
  },
  entry: {
    index: './src/index.js',
  },
  module: {
    rules: [
      {
        test: /\.vue$/,
        use: 'vue-loader'
      },
      {
        test: /\.css$/,
        use: [
          { loader: "vue-style-loader" },
          { loader: "null-loader" },      // <------------- add this instead
//        { loader: MiniCssExtractPlugin.loader, options: { esModule: false } },
//        { loader: "css-loader" }
        ]
      }
    ]
  },
  plugins: [
     new webpack.DefinePlugin({
      COMPILE_TARGET: JSON.stringify('node')
    }),
    new VueLoaderPlugin(),
//  new MiniCssExtractPlugin({
//    filename: '[name].css'
//  })
  ],
  devtool: false
}

It would be nice to have it tho, meaning the same plugin / loader could be used in either case with a single option being the difference.

Also implementation is invalid, because we don't need process something in loader, we can just return empty content

Can you elaborate?

[ferdinando-ferreira]

@alexander-akait : from what I understand your suggestion would be

• don't use the plugin for SSR
• just pass it to null-loader to remove from the final javascript

That would be it?

[ferdinando-ferreira]

Closing the PR because from what I've understood it is not implemented correctly and doesn't merit further discussion.

Also opened webpack/webpack#12920 because, with null-loader being deprecated, even this won't work in future versions.

[alexander-akait]

You don't need null-loader, you can do using resolve

[ferdinando-ferreira]

you can do using resolve

@alexander-akait : Is it possible to do that without knowing at configuration time the name of the files to be ignored? Documentation says you have to ignore by name using resolve.alias

[alexander-akait]

webpack/webpack#12920 is not solve your problem, CSS modules will not work

[alexander-akait]

Let's reopen, after investigate I found what locals can be broken with null

[ferdinando-ferreira]

Reopened and it works and passes all tests. All that's missing

don't need process something in loader, we can just return empty content

right?

[alexander-akait]

Let's move it on loader level, need just avoid https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/loader.js#L193 and https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/src/loader.js#L157 when emit false and let's rename option to emit

[codecov]

Codecov Report

Merging #722 (5390926) into master (6ae4c3e) will decrease coverage by 19.27%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #722       +/-   ##
===========================================
- Coverage   89.67%   70.39%   -19.28%     
===========================================
  Files           6        6               
  Lines         775      777        +2     
  Branches      239      241        +2     
===========================================
- Hits          695      547      -148     
- Misses         77      212      +135     
- Partials        3       18       +15     

Impacted Files	Coverage Δ
src/loader.js	82.87% <100.00%> (-9.49%)	⬇️
src/utils.js	53.84% <0.00%> (-34.62%)	⬇️
src/index.js	60.89% <0.00%> (-27.14%)	⬇️

---

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 6ae4c3e...5390926. Read the comment docs.

[ferdinando-ferreira]

let's rename option to emit

Done. Will move the logic from the plugin to the loader as requested

[ferdinando-ferreira]

@alexander-akait : done, it works.

You can check with

git clone https://github.com/ferdinando-ferreira/mini-css-extract-plugin --single-branch mini-css-extract-plugin
cd mini-css-extract-plugin
npm install
cd usecase
npm install
npm run build

• usecase\dist\current-plugin will have index.css on ssr folder
• usecase\dist\intended-behaviour will not have index.css on ssr folder

The only difference in the config is emit: false.

It also does not erase locals like null-loader would.

[ferdinando-ferreira]

@alexander-akait : before reviewing, let me create an usecase for locals to ensure it works as intended in both cases

[ferdinando-ferreira]

@alexander-akait : done!

To test all usecases and the new one for locals:

git clone https://github.com/ferdinando-ferreira/mini-css-extract-plugin --single-branch mini-css-extract-plugin
cd mini-css-extract-plugin
npm install
cd usecase
npm install
npm run build

Compilation result will be at usecase/dist/.

Then to test the version with locals

npm run start:testing-with-locals

The result must be

{ a: 'foo__style__a', b: 'foo__style__b' }

Which is the same result from the example this was copied from: https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/commonjs-module-syntax/webpack.config.js

Please review and validate.

[ferdinando-ferreira]

@alexander-akait , @cap-Bernardito : I assume #732 is a more elegant way to implement this, thanks for merging.