extractComments only works sometimes

[jahed]

• Operating System: Fedora 32
• Node Version: 12
• NPM Version: 6
• webpack Version: 4
• terser-webpack-plugin Version: 4.2.0

Expected Behavior

extractComments to extract and add a banner always.

Actual Behavior

extractComments doesn't reliably add the banner. Sometimes, maybe 50% of the time, it will remove comments, but won't add the banner. The [contenthash] is the same regardless which causes caching and SRI issues as the hash does not reflect the content without the banner.

Code

I can try to provide code, but I noticed the tests for extractComments is failing so possibly that will help.

https://github.com/webpack-contrib/terser-webpack-plugin/runs/1103414191?check_suite_focus=true

FAIL test/minify-option.test.js
  ● Console

    console.log
      Current webpack version: 4

      at Object.<anonymous> (test/helpers/snapshotResolver.js:10:9)

  ● minify option › should snapshot with extracting comments

    EINVAL: invalid argument, rmdir '/Users/runner/work/terser-webpack-plugin/terser-webpack-plugin/node_modules/.cache/terser-webpack-plugin/content-v2/sha512/12/ec'

How Do We Reproduce?

See above.

[alexander-akait]

@jahed please create reproducible test repo, this errors from CI related only for macos and unstable watch due macos bugs

[alexander-akait]

I will reopen after minimum reproducible test repo, I can't reproduce so I can't fix, sorry

[alexander-akait]

Banner changes contenthash https://github.com/webpack-contrib/terser-webpack-plugin/blob/master/src/index.js#L628 always, changing any terser options always generate other hash

[jahed]

It's a caching issue. The cached file has no comment line when it should.

Repro:

https://github.com/jahed/terser-webpack-plugin-extract-comments-issue

Run npm test and check the output folder. Folders are named by timestamp of
build. The first folder has the the comment line, while other folders do not
even though they have the same chunkhash. This is because the cache under
node_modules/.cache/terser-webpack-plugin caches the content without comments.

[jahed]

I have also noticed comment blocks aren't being extracted. See the repro, 2 dep files with license comment blocks (using /*!), none of them are in the output so I had to use extractComments: 'all' to get some comments extracted. But this may be a separate issue.

[alexander-akait]

CI tests still not related to the problem, here racing in cache in tests, I will fix it in near future

[alexander-akait]

I have also noticed comment blocks aren't being extracted. See the repro, 2 dep files with license comment blocks (using /*!), none of them are in the output so I had to use extractComments: 'all' to get some comments extracted. But this may be a separate issue.

Looks like bug on terser side, because we don't accept them in https://github.com/webpack-contrib/terser-webpack-plugin/blob/master/src/minify.js#L127

[alexander-akait]

It's a caching issue. The cached file has no comment line when it should.

Bug, WIP

[jahed]

I tried going through the code to send a PR but it's difficult to know if I will break any other behaviour so I'll wait for a fix.

I believe the bug is here to do with this area of the code:

terser-webpack-plugin/src/index.js

Lines 431 to 435 in defde64

	output.source = new ConcatSource(
	shebang ? `${shebang}\n` : '',
	`/*! ${banner} */\n`,
	output.source
	);

output.source is assigned the new source with the banner. However, in cache.store, the cachedData.source isn't copied into the data so it is never cached.

terser-webpack-plugin/src/Webpack4Cache.js

Lines 102 to 108 in defde64

	data = {
	code: cacheData.code,
	name: cacheData.name,
	map: cacheData.map,
	input: cacheData.input,
	inputSourceMap: cacheData.inputSourceMap,
	};

In mean time I'll stay on 4.1.0.

Thanks for looking into this. 👍🏽

[alexander-akait]

Found bug, patch release will be tomorrow

[alexander-akait]

Problem with banner fixed, investigate why terser does not handle some comments

[jahed]

I did some quick investigation. I didn't get the time to create a repro and file an issue in Terser's repo yet.

It does seem to be a Terser issue.

• Using CLI with non-minified Webpack output, the /*! comments are kept.
• Using the API (require('terser')) without any custom options, the comments are removed.
• Using output.comments function, the /*! comments aren't received.
• Using output.comments with /^!/ RegExp, the comments are removed.
• Using it in CLI using --comments '/^!/', the comments are kept

Also tried with format.comments since output.comments is legacy but behaviour didn't change (it fallbacks to output.comments anyway).

So there seems to be an issue with the API's implementation. CLI behaves correctly.

[alexander-akait]

@jahed I think here no issues, because we concatenate modules original modules were destroyed, so we don't need to preserve/extract comments

[alexander-akait]

Change code on:

/*!
 * @preserve
 * My License Block with new line
 */
doSomething(helloWorld)

And you will get the extracted comment.

[alexander-akait]

webpack + terser change:

doSomething(helloWorld)

to

console.log('Hello, world!');

So original modules doesn't exists and no comments for this

[alexander-akait]

More verbose example with minimize: false

/******/ (function(modules) { // webpackBootstrap
/******/ 	// The module cache
/******/ 	var installedModules = {};
/******/
/******/ 	// The require function
/******/ 	function __webpack_require__(moduleId) {
/******/
/******/ 		// Check if module is in cache
/******/ 		if(installedModules[moduleId]) {
/******/ 			return installedModules[moduleId].exports;
/******/ 		}
/******/ 		// Create a new module (and put it into the cache)
/******/ 		var module = installedModules[moduleId] = {
/******/ 			i: moduleId,
/******/ 			l: false,
/******/ 			exports: {}
/******/ 		};
/******/
/******/ 		// Execute the module function
/******/ 		modules[moduleId].call(module.exports, module, module.exports, __webpack_require__);
/******/
/******/ 		// Flag the module as loaded
/******/ 		module.l = true;
/******/
/******/ 		// Return the exports of the module
/******/ 		return module.exports;
/******/ 	}
/******/
/******/
/******/ 	// expose the modules object (__webpack_modules__)
/******/ 	__webpack_require__.m = modules;
/******/
/******/ 	// expose the module cache
/******/ 	__webpack_require__.c = installedModules;
/******/
/******/ 	// define getter function for harmony exports
/******/ 	__webpack_require__.d = function(exports, name, getter) {
/******/ 		if(!__webpack_require__.o(exports, name)) {
/******/ 			Object.defineProperty(exports, name, { enumerable: true, get: getter });
/******/ 		}
/******/ 	};
/******/
/******/ 	// define __esModule on exports
/******/ 	__webpack_require__.r = function(exports) {
/******/ 		if(typeof Symbol !== 'undefined' && Symbol.toStringTag) {
/******/ 			Object.defineProperty(exports, Symbol.toStringTag, { value: 'Module' });
/******/ 		}
/******/ 		Object.defineProperty(exports, '__esModule', { value: true });
/******/ 	};
/******/
/******/ 	// create a fake namespace object
/******/ 	// mode & 1: value is a module id, require it
/******/ 	// mode & 2: merge all properties of value into the ns
/******/ 	// mode & 4: return value when already ns object
/******/ 	// mode & 8|1: behave like require
/******/ 	__webpack_require__.t = function(value, mode) {
/******/ 		if(mode & 1) value = __webpack_require__(value);
/******/ 		if(mode & 8) return value;
/******/ 		if((mode & 4) && typeof value === 'object' && value && value.__esModule) return value;
/******/ 		var ns = Object.create(null);
/******/ 		__webpack_require__.r(ns);
/******/ 		Object.defineProperty(ns, 'default', { enumerable: true, value: value });
/******/ 		if(mode & 2 && typeof value != 'string') for(var key in value) __webpack_require__.d(ns, key, function(key) { return value[key]; }.bind(null, key));
/******/ 		return ns;
/******/ 	};
/******/
/******/ 	// getDefaultExport function for compatibility with non-harmony modules
/******/ 	__webpack_require__.n = function(module) {
/******/ 		var getter = module && module.__esModule ?
/******/ 			function getDefault() { return module['default']; } :
/******/ 			function getModuleExports() { return module; };
/******/ 		__webpack_require__.d(getter, 'a', getter);
/******/ 		return getter;
/******/ 	};
/******/
/******/ 	// Object.prototype.hasOwnProperty.call
/******/ 	__webpack_require__.o = function(object, property) { return Object.prototype.hasOwnProperty.call(object, property); };
/******/
/******/ 	// __webpack_public_path__
/******/ 	__webpack_require__.p = "";
/******/
/******/
/******/ 	// Load entry module and return exports
/******/ 	return __webpack_require__(__webpack_require__.s = 0);
/******/ })
/************************************************************************/
/******/ ([
/* 0 */
/***/ (function(module, __webpack_exports__, __webpack_require__) {

"use strict";
// ESM COMPAT FLAG
__webpack_require__.r(__webpack_exports__);

// CONCATENATED MODULE: ./dep-new-line.js
/*!
 * @preserve
 * My License Block with new line
 */

const doSomething = s => {
    console.log(s)
}

// CONCATENATED MODULE: ./dep-without-new-line.js
/*!
 * My License Block
 */
const helloWorld = 'Hello, world!'

// CONCATENATED MODULE: ./index.js



doSomething(helloWorld)


/***/ })
/******/ ]);

But terser:

• replace doSomething on console.log and remove the doSomething function with leading comments
• replace helloWorld' on 'Hello, world!'and remove thehelloWorld` variable with leading comments

So nothing to extract or preserve

[alexander-akait]

This is expected behavior, although it may be a little unclear, that's why comments usually put on top of the main file (i.e index.js), tree shacking can remove whole functions/files, and you will lost comments

[jahed]

It's weird that CLI preserves /*! by default but API doesn't.

# Same as https://gist.github.com/jahed/9983dea6c0d992581e2a3cf53a67e482
npx terser output/1600172776224/6d78486641ca06d33d76.js

(function(modules){var installedModules={};function __webpack_require__(moduleId){if(installedModules[moduleId]){return installedModules[moduleId].exports}var module=installedModules[moduleId]={i:moduleId,l:false,exports:{}};modules[moduleId].call(module.exports,module,module.exports,__webpack_require__);module.l=true;return module.exports}__webpack_require__.m=modules;__webpack_require__.c=installedModules;__webpack_require__.d=function(exports,name,getter){if(!__webpack_require__.o(exports,name)){Object.defineProperty(exports,name,{enumerable:true,get:getter})}};__webpack_require__.r=function(exports){if(typeof Symbol!=="undefined"&&Symbol.toStringTag){Object.defineProperty(exports,Symbol.toStringTag,{value:"Module"})}Object.defineProperty(exports,"__esModule",{value:true})};__webpack_require__.t=function(value,mode){if(mode&1)value=__webpack_require__(value);if(mode&8)return value;if(mode&4&&typeof value==="object"&&value&&value.__esModule)return value;var ns=Object.create(null);__webpack_require__.r(ns);Object.defineProperty(ns,"default",{enumerable:true,value:value});if(mode&2&&typeof value!="string")for(var key in value)__webpack_require__.d(ns,key,function(key){return value[key]}.bind(null,key));return ns};__webpack_require__.n=function(module){var getter=module&&module.__esModule?function getDefault(){return module["default"]}:function getModuleExports(){return module};__webpack_require__.d(getter,"a",getter);return getter};__webpack_require__.o=function(object,property){return Object.prototype.hasOwnProperty.call(object,property)};__webpack_require__.p="";return __webpack_require__(__webpack_require__.s=0)})([function(module,__webpack_exports__,__webpack_require__){"use strict";__webpack_require__.r(__webpack_exports__);
/*! My License Block with new line */const doSomething=s=>{console.log(s)};
/*!
 * My License Block
 */const helloWorld="Hello, world!";doSomething(helloWorld)}]);

This is unrelated to terser-webpack-plugin so feel free to close this issue.

I've seen similar issues in terser about this, and they agree it's a non issue so I'll leave it at that. This isn't an issue.

[alexander-akait]

@jahed when you run npx terser you don't setup options for terser, you can see helloWorld and doSomething were not inlined, that's why the comments were saved, I am not very familiar with terser CLI, but there seems to be a difference between CLI and Node.js API, but yes, this is not our problem, thanks for the issue