Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug placeholders replaced with colors in production #21489

Closed
sirreal opened this issue Jan 12, 2018 · 5 comments · Fixed by #23388
Closed

Debug placeholders replaced with colors in production #21489

sirreal opened this issue Jan 12, 2018 · 5 comments · Fixed by #23388

Comments

@sirreal
Copy link
Member

sirreal commented Jan 12, 2018

In NODE_ENV === 'production' builds, the %s string placeholder can be observed being replaced with colors rather than the provided value. I haven't verified whether other placeholders work as expected.

Steps to reproduce

  1. Visit https://wordpress.com
  2. Type the following in the browser console.
    localStorage.debug = 'calypso:popover'
  3. Refresh and observe color nonsense in the debug statements.

What I expected

%s should be replaced by the provided arguments.

What happened instead

%s is replaced by what appears to be the color for the current debug instance.

Screenshot / Video

Dev

dev

Prod

prod

Context / Source

Spotted while shipping #21104 which led to workaround #21390

/cc @samouri (#12841)

@apeatling apeatling removed [Pri] Normal Schedule for the next available opportuinity. [Pri] Low Address when resources are available. labels Feb 1, 2018
@sirreal
Copy link
Member Author

sirreal commented Mar 7, 2018

Naïvely tried upgrading these modules with no success:

  • babel-core
  • babel-loader
  • babel-preset-env
  • babel-register
  • debug
  • uglifyjs-webpack-plugin

@sirreal
Copy link
Member Author

sirreal commented Mar 16, 2018

I've narrowed this down to uglification. With this patch, I no longer see the issue in Docker builds:

diff --git a/webpack.config.js b/webpack.config.js
index fcdf23efa5..61655855cc 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -31,9 +31,7 @@ const config = require( './server/config' );
 const calypsoEnv = config( 'env_id' );
 const bundleEnv = config( 'env' );
 const isDevelopment = bundleEnv === 'development';
-const shouldMinify = process.env.hasOwnProperty( 'MINIFY_JS' )
-	? process.env.MINIFY_JS === 'true'
-	: ! isDevelopment;
+const shouldMinify = false;
 
 // load in the babel config from babelrc and disable commonjs transform
 // this enables static analysis from webpack including treeshaking

@sirreal
Copy link
Member Author

sirreal commented Mar 16, 2018

Uglify compress is the culprit. With this patch there's no issue:

diff --git a/webpack.config.js b/webpack.config.js
index fcdf23efa5..d77beea979 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -295,7 +295,7 @@ if ( shouldMinify ) {
 		new UglifyJsPlugin( {
 			cache: 'docker' !== process.env.CONTAINER,
 			parallel: true,
-			uglifyOptions: { ecma: 5 },
+			uglifyOptions: { compress: false, ecma: 5 },
 			sourceMap: Boolean( process.env.SOURCEMAP ),
 		} )
 	);

@sirreal
Copy link
Member Author

sirreal commented Mar 17, 2018

Seems to be a bug in uglify-es: mishoo/UglifyJS#3010 (thanks @ockham 💪)

@blowery
Copy link
Contributor

blowery commented Mar 17, 2018

You two. Amazing detective work! 🔍

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

Successfully merging a pull request may close this issue.

4 participants