feat: emitFile option #722

ferdinando-ferreira · 2021-03-17T15:03:40Z

This PR contains a:

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

Add emitFile 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. It is often useful to disable this option for server-side packages.

alexander-akait · 2021-03-17T15:13:38Z

Why you need this plugin for SSR?

ferdinando-ferreira · 2021-03-17T15:16:15Z

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 · 2021-03-17T15:21:56Z

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

ferdinando-ferreira · 2021-03-17T15:35:05Z

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 · 2021-03-17T15:36:40Z

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

ferdinando-ferreira · 2021-03-17T15:41:40Z

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 · 2021-03-17T15:47:36Z

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 · 2021-03-17T15:49:59Z

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

ferdinando-ferreira · 2021-03-17T16:03:19Z

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 · 2021-03-17T16:08:06Z

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 · 2021-03-17T16:34:39Z

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 · 2021-03-17T16:55:08Z

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 · 2021-03-17T17:14:10Z

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 · 2021-03-17T17:29:15Z

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 · 2021-03-18T12:34:12Z

@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 · 2021-03-18T12:35:46Z

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 · 2021-03-18T12:36:51Z

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

ferdinando-ferreira · 2021-03-18T12:38:44Z

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 · 2021-03-18T14:34:49Z

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

alexander-akait · 2021-03-18T15:00:20Z

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

ferdinando-ferreira · 2021-03-18T16:04:06Z

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

Renamed from emitFile

codecov · 2021-03-18T16:46:39Z

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 · 2021-03-18T16:47:51Z

let's rename option to emit

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

ferdinando-ferreira · 2021-03-18T23:39:31Z

@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.

Removed the unnecessary check. Codecov was complaining about it and there is only one caller for this function a few lines below, dependencies is guaranteed to be an array

ferdinando-ferreira · 2021-03-19T10:51:51Z

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

Keep addDependencies to ensure watch works

Restoring mistakenly removed formatting

ferdinando-ferreira · 2021-03-19T20:10:20Z

@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 · 2021-03-26T19:07:04Z

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

feat: emitFile option

34ac7ad

Add emitFile 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. It is often useful to disable this option for server-side packages.

ferdinando-ferreira mentioned this pull request Mar 17, 2021

New option: EmitFile, similar to file-loader to allow for the plugin ***not*** to emit the assets #713

Closed

ferdinando-ferreira closed this Mar 18, 2021

ferdinando-ferreira deleted the emitFiles branch March 18, 2021 12:36

ferdinando-ferreira restored the emitFiles branch March 18, 2021 16:02

ferdinando-ferreira reopened this Mar 18, 2021

alexander-akait reviewed Mar 18, 2021

View reviewed changes

feat: emit option

350ba1a

Renamed from emitFile

feat: emit option, moved to loader

0cbea18

feat: emit option

46a219c

Removed the unnecessary check. Codecov was complaining about it and there is only one caller for this function a few lines below, dependencies is guaranteed to be an array

ferdinando-ferreira added 2 commits March 19, 2021 12:29

feat: emit option

d6ff0bc

Keep addDependencies to ensure watch works

feat: emit option

5390926

Restoring mistakenly removed formatting

cap-Bernardito mentioned this pull request Mar 26, 2021

Feat emit option #732

Merged

6 tasks

alexander-akait closed this in #732 Mar 26, 2021

ferdinando-ferreira deleted the emitFiles branch March 26, 2021 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: emitFile option #722

feat: emitFile option #722

ferdinando-ferreira commented Mar 17, 2021 •

edited

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021 •

edited

alexander-akait commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

ferdinando-ferreira commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021

alexander-akait commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021 •

edited

alexander-akait commented Mar 18, 2021

alexander-akait commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021

alexander-akait left a comment

codecov bot commented Mar 18, 2021 •

edited

ferdinando-ferreira commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021

ferdinando-ferreira commented Mar 19, 2021

ferdinando-ferreira commented Mar 19, 2021

ferdinando-ferreira commented Mar 26, 2021

feat: emitFile option #722

feat: emitFile option #722

Conversation

ferdinando-ferreira commented Mar 17, 2021 • edited

Motivation / Use-Case

Breaking Changes

Additional Info

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021 • edited

alexander-akait commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

alexander-akait commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

ferdinando-ferreira commented Mar 17, 2021

ferdinando-ferreira commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021

alexander-akait commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021 • edited

alexander-akait commented Mar 18, 2021

alexander-akait commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021

alexander-akait left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2021 • edited

Codecov Report

ferdinando-ferreira commented Mar 18, 2021

ferdinando-ferreira commented Mar 18, 2021

ferdinando-ferreira commented Mar 19, 2021

ferdinando-ferreira commented Mar 19, 2021

ferdinando-ferreira commented Mar 26, 2021

ferdinando-ferreira commented Mar 17, 2021 •

edited

ferdinando-ferreira commented Mar 17, 2021 •

edited

ferdinando-ferreira commented Mar 18, 2021 •

edited

codecov bot commented Mar 18, 2021 •

edited