webpack-terser-plugin slices console calls and shaka crashes

[alexandercerutti]

Have you read the FAQ and checked for duplicate open issues?

Yes

What version of Shaka Player are you using?

v3.3.15, but the code is the same in v4

Can you reproduce the issue with our latest release version?

Yes

Can you reproduce the issue with the latest code from main?

I guess so, didn't test

Are you using the demo app or your own custom app?

Custom app

If custom app, can you reproduce the issue using our demo app?

Not needed

What browser and OS are you using?

Whatever modern browser is good for this issue

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

_

What are the manifest and license server URIs?

What configuration are you using? What is the output of player.getConfiguration()?

What did you do?

See discussion below

What did you expect to happen?

See discussion below

What actually happened?

In our custom application, we are adding webpack-terser-plugin, which uses https://github.com/terser/terser under the hood.

We have a lot of console.logs that we want to remove in the production build. Therefore we set in Webpack the following configuration:

minimizer: [
	new TerserPlugin({
		terserOptions: {
			compress: {
				drop_console: true,
			},
		},
	}),
],

This configuration trims all the console.*, even those in shaka.

We were experiencing an error which we investigated up and found out to be something like:

Uncaught TypeError: shaka.log.alwaysWarn is not a function

Minified something like this:

Uncaught TypeError: z is not a function

This was being triggered by the fact that through shaka v3 we are still using addTextTrack instead of addTextTrackAsync, so we were actually able to fix this on our side, but this issue might still happen with other methods, theoretically.

Why does it happen? We found out the responsible is this piece of code, available both on v3 and v4:

shaka-player/lib/debug/log.js

Lines 137 to 155 in c7c5e94

	// IE8 has no console unless it is opened in advance.
	// IE9 console methods are not Functions and have no bind.
	if (window.console && window.console.log.bind) {
	/** @private {!Object.<shaka.log.Level, function(...*)>} */
	shaka.log.logMap_ = {
	/* eslint-disable no-restricted-syntax */
	[shaka.log.Level.ERROR]: console.error.bind(console),
	[shaka.log.Level.WARNING]: console.warn.bind(console),
	[shaka.log.Level.INFO]: console.info.bind(console),
	[shaka.log.Level.DEBUG]: console.log.bind(console),
	[shaka.log.Level.V1]: console.debug.bind(console),
	[shaka.log.Level.V2]: console.debug.bind(console),
	/* eslint-enable no-restricted-syntax */
	};
	shaka.log.alwaysWarn = shaka.log.logMap_[shaka.log.Level.WARNING];
	shaka.log.alwaysError = shaka.log.logMap_[shaka.log.Level.ERROR];
	if (goog.DEBUG) {

Which, I guess is done to let deprecation warnings to always get shown.
The issue is that, when using webpack-terser-plugin, that code becomes this.

First image is the compiled one:

Second image is after terser compilation:

So, it is clear that z received undefined as value and when shaka.log.alwaysWarn is invoked, this crashes because e.console (so, window.console) still exists, but the bindings were actually removed.

Thank you.

P.s. why do bindings still exist if it was a workaround for IE 8 and 9, if they are not supported anymore?

/cc @salvatore-esposito-green (my colleague)

[martinstark]

I think it's up for debate whether this is a bug or not and I'm curious about the Shaka teams view on this. I would say that, when stripping code out of pre-bundled dependencies, unexpected behaviour should be expected. I believe the norm is to not include dependencies in transpilation when they are already bundled/transpiled.

I think it's reasonable to want to remove all console logs from production environments, so in that sense it feels like a bug.

In the mean time, Terser accepts an exclude pattern that can be used (as I'm sure you're aware of):

   module.exports = {
      optimization: {
        minimizer: [
          new TerserPlugin({
            exclude: /node_modules/, // or path to shaka package
          }),
        ],
      },
    };

[alexandercerutti]

@martinstark for sure it is debatable. We knew exclude but maybe something might have gone wrong, I don't know. Anyway, I preferred opening this issue so that, for future reference, anyone could get benefit from whatever will come out of this by just looking in the issues.

I flagged this as a bug because it is not a feature nor a question, so the most suitable was it.

I'm curious too about the Shaka team's view on this.

[theodab]

In the general case I'm not sure if we should be changing our code so that it works better with tools that post-process the code. In this case though, it seems like a fairly simple fix so we might as well do it.

P.s. why do bindings still exist if it was a workaround for IE 8 and 9, if they are not supported anymore?

Because we didn't think to check the log function when deprecating IE, probably. Might as well change that at the same time.

[joeyparrish]

In the general case I'm not sure if we should be changing our code so that it works better with tools that post-process the code.

I disagree. Interoperability is good, and our single bundle of JS is increasingly unusual in the ecosystem as everyone moves to modules and TypeScript.

Thanks for working on the fix, @theodab!

[joeyparrish]

Also, one thing that is easy to mistake here is that we never supported IE 8 and 9 for Shaka Player, but the bundle was designed not to throw at load-time on those older browsers (we began the project in 2014) so that we could at least return false from isBrowserSupported(). I don't think it matters now, though.