From a9a37e332de232737d286495cb40a451f3eee759 Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Mon, 24 Oct 2016 15:30:20 -0700 Subject: [PATCH 1/5] Fix for #7: Not as optimized for Webpack as it could be. Adds ability to wrap functions that are elements of an array literal in an argument list --- lib/index.js | 69 +++++++++++++------ .../webpack-style-multi-element/input.js | 9 +++ .../webpack-style-multi-element/output.js | 9 +++ test/cases/webpack-style-one-element/input.js | 5 ++ .../cases/webpack-style-one-element/output.js | 5 ++ 5 files changed, 75 insertions(+), 22 deletions(-) create mode 100644 test/cases/webpack-style-multi-element/input.js create mode 100644 test/cases/webpack-style-multi-element/output.js create mode 100644 test/cases/webpack-style-one-element/input.js create mode 100644 test/cases/webpack-style-one-element/output.js diff --git a/lib/index.js b/lib/index.js index 5e4d3c8..be4ad22 100644 --- a/lib/index.js +++ b/lib/index.js @@ -21,15 +21,16 @@ function optimizeJs (jsString, opts) { var postChar = jsString.charAt(node.end) var postPostChar = jsString.charAt(node.end + 1) - // assuming this node is an argument to a function, return true if it itself - // is already padded with parentheses + // assuming this node is an argument to a function or an element in an array, + // return true if it itself is already padded with parentheses function isPaddedArgument (node) { - var idx = node.parentNode.arguments.indexOf(node) + var parentArray = node.parentNode.arguments ? node.parentNode.arguments : node.parentNode.elements + var idx = parentArray.indexOf(node) if (idx === 0) { // first arg if (prePreChar === '(' && preChar === '(' && postChar === ')') { // already padded return true } - } else if (idx === node.parentNode.arguments.length - 1) { // last arg + } else if (idx === parentArray.length - 1) { // last arg if (preChar === '(' && postChar === ')' && postPostChar === ')') { // already padded return true } @@ -41,24 +42,48 @@ function optimizeJs (jsString, opts) { return false } - if (node.parentNode && node.parentNode.type === 'CallExpression') { - // this function is getting called itself or - // it is getting passed in to another call expression - // the else statement is strictly never hit, but I think the code is easier to read this way - /* istanbul ignore else */ - if (node.parentNode.arguments.length && node.parentNode.arguments.indexOf(node) !== -1) { - // function passed in to another function. these are almost _always_ executed, e.g. - // UMD bundles, Browserify bundles, Node-style errbacks, Promise then()s and catch()s, etc. - if (!isPaddedArgument(node)) { // don't double-pad - magicString = magicString.insertLeft(node.start, '(') - .insertRight(node.end, ')') - } - } else if (node.parentNode.callee === node) { - // this function is getting immediately invoked, e.g. function(){}() - if (preChar !== '(') { - magicString.insertLeft(node.start, '(') - .insertRight(node.end, ')') - } + function isCallExpression (node) { + return node && node.type === 'CallExpression' + } + + function isArrayExpression (node) { + return node && node.type === 'ArrayExpression' + } + + // returns true iff node is an argument to a function call expression. + function isArgumentToFunctionCall (node) { + return isCallExpression(node.parentNode) && + node.parentNode.arguments.length && + node.parentNode.arguments.indexOf(node) !== -1 + } + + // returns true iff node is an element of an array literal which is in turn + // an argument to a function call expression. + function isElementOfArrayArgumentToFunctionCall (node) { + return isArrayExpression(node.parentNode) && + node.parentNode.elements.indexOf(node) !== -1 && + isArgumentToFunctionCall(node.parentNode) + } + + // returns true iff node is an IIFE. + function isIIFE (node) { + return isCallExpression(node.parentNode) && + node.parentNode.callee === node + } + + if (isArgumentToFunctionCall(node) || isElementOfArrayArgumentToFunctionCall(node)) { + // function passed in to another function, either as an argument, or as an element + // of an array argument. these are almost _always_ executed, e.g. webpack bundles, + // UMD bundles, Browserify bundles, Node-style errbacks, Promise then()s and catch()s, etc. + if (!isPaddedArgument(node)) { // don't double-pad + magicString = magicString.insertLeft(node.start, '(') + .insertRight(node.end, ')') + } + } else if (isIIFE(node)) { + // this function is getting immediately invoked, e.g. function(){}() + if (preChar !== '(') { + magicString.insertLeft(node.start, '(') + .insertRight(node.end, ')') } } } diff --git a/test/cases/webpack-style-multi-element/input.js b/test/cases/webpack-style-multi-element/input.js new file mode 100644 index 0000000..1ee809f --- /dev/null +++ b/test/cases/webpack-style-multi-element/input.js @@ -0,0 +1,9 @@ +!function(o){ + return o[0](); +}([function(o,r,t){ + console.log("webpack style element 0"); +},function(o,r,t){ + console.log("webpack style element 1"); +},function(o,r,t){ + console.log("webpack style element 2"); +}]); diff --git a/test/cases/webpack-style-multi-element/output.js b/test/cases/webpack-style-multi-element/output.js new file mode 100644 index 0000000..31b5b09 --- /dev/null +++ b/test/cases/webpack-style-multi-element/output.js @@ -0,0 +1,9 @@ +!(function(o){ + return o[0](); +})([(function(o,r,t){ + console.log("webpack style element 0"); +}),(function(o,r,t){ + console.log("webpack style element 1"); +}),(function(o,r,t){ + console.log("webpack style element 2"); +})]); diff --git a/test/cases/webpack-style-one-element/input.js b/test/cases/webpack-style-one-element/input.js new file mode 100644 index 0000000..6758239 --- /dev/null +++ b/test/cases/webpack-style-one-element/input.js @@ -0,0 +1,5 @@ +!function(o){ + return o[0](); +}([function(o,r,t){ + console.log("webpack style!"); +}]); diff --git a/test/cases/webpack-style-one-element/output.js b/test/cases/webpack-style-one-element/output.js new file mode 100644 index 0000000..b46979a --- /dev/null +++ b/test/cases/webpack-style-one-element/output.js @@ -0,0 +1,5 @@ +!(function(o){ + return o[0](); +})([(function(o,r,t){ + console.log("webpack style!"); +})]); From 4895d38fb4c57007d015505d72a4cf70b98ea087 Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Sat, 29 Oct 2016 10:40:25 -0700 Subject: [PATCH 2/5] Added a few negative case tests in response to code review. --- test/cases/array-literal-iife-in-function-scope/input.js | 5 +++++ test/cases/array-literal-iife-in-function-scope/output.js | 5 +++++ test/cases/array-literal-iife-in-global-scope/input.js | 3 +++ test/cases/array-literal-iife-in-global-scope/output.js | 3 +++ test/cases/array-literal-in-function-scope/input.js | 5 +++++ test/cases/array-literal-in-function-scope/output.js | 5 +++++ test/cases/array-literal-in-global-scope/input.js | 3 +++ test/cases/array-literal-in-global-scope/output.js | 3 +++ 8 files changed, 32 insertions(+) create mode 100644 test/cases/array-literal-iife-in-function-scope/input.js create mode 100644 test/cases/array-literal-iife-in-function-scope/output.js create mode 100644 test/cases/array-literal-iife-in-global-scope/input.js create mode 100644 test/cases/array-literal-iife-in-global-scope/output.js create mode 100644 test/cases/array-literal-in-function-scope/input.js create mode 100644 test/cases/array-literal-in-function-scope/output.js create mode 100644 test/cases/array-literal-in-global-scope/input.js create mode 100644 test/cases/array-literal-in-global-scope/output.js diff --git a/test/cases/array-literal-iife-in-function-scope/input.js b/test/cases/array-literal-iife-in-function-scope/input.js new file mode 100644 index 0000000..0b4de12 --- /dev/null +++ b/test/cases/array-literal-iife-in-function-scope/input.js @@ -0,0 +1,5 @@ +function b() { + var a = [ + !function() {return 1;}() + ]; +} diff --git a/test/cases/array-literal-iife-in-function-scope/output.js b/test/cases/array-literal-iife-in-function-scope/output.js new file mode 100644 index 0000000..a8156e4 --- /dev/null +++ b/test/cases/array-literal-iife-in-function-scope/output.js @@ -0,0 +1,5 @@ +function b() { + var a = [ + !(function() {return 1;})() + ]; +} diff --git a/test/cases/array-literal-iife-in-global-scope/input.js b/test/cases/array-literal-iife-in-global-scope/input.js new file mode 100644 index 0000000..ad9f3a4 --- /dev/null +++ b/test/cases/array-literal-iife-in-global-scope/input.js @@ -0,0 +1,3 @@ +var a = [ + !function() {return 1;}() +] diff --git a/test/cases/array-literal-iife-in-global-scope/output.js b/test/cases/array-literal-iife-in-global-scope/output.js new file mode 100644 index 0000000..1cbef83 --- /dev/null +++ b/test/cases/array-literal-iife-in-global-scope/output.js @@ -0,0 +1,3 @@ +var a = [ + !(function() {return 1;})() +] diff --git a/test/cases/array-literal-in-function-scope/input.js b/test/cases/array-literal-in-function-scope/input.js new file mode 100644 index 0000000..d9a89be --- /dev/null +++ b/test/cases/array-literal-in-function-scope/input.js @@ -0,0 +1,5 @@ +function b() { + var a = [ + function() {return 1;} + ]; +} diff --git a/test/cases/array-literal-in-function-scope/output.js b/test/cases/array-literal-in-function-scope/output.js new file mode 100644 index 0000000..d9a89be --- /dev/null +++ b/test/cases/array-literal-in-function-scope/output.js @@ -0,0 +1,5 @@ +function b() { + var a = [ + function() {return 1;} + ]; +} diff --git a/test/cases/array-literal-in-global-scope/input.js b/test/cases/array-literal-in-global-scope/input.js new file mode 100644 index 0000000..65e8d4c --- /dev/null +++ b/test/cases/array-literal-in-global-scope/input.js @@ -0,0 +1,3 @@ +var a = [ + function() {return 1;} +] diff --git a/test/cases/array-literal-in-global-scope/output.js b/test/cases/array-literal-in-global-scope/output.js new file mode 100644 index 0000000..65e8d4c --- /dev/null +++ b/test/cases/array-literal-in-global-scope/output.js @@ -0,0 +1,3 @@ +var a = [ + function() {return 1;} +] From aa9efd3c0c700a6b6908ad5fbbfa0e85f4ebb098 Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Sat, 29 Oct 2016 13:16:15 -0700 Subject: [PATCH 3/5] Modified the array literal logic to very specifically track with webpack patterns rather than all array literal function parameters. --- lib/index.js | 14 +++++++++++++- .../array-literal-in-function-params/input.js | 5 +++++ .../array-literal-in-function-params/output.js | 5 +++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 test/cases/array-literal-in-function-params/input.js create mode 100644 test/cases/array-literal-in-function-params/output.js diff --git a/lib/index.js b/lib/index.js index be4ad22..21b2e5c 100644 --- a/lib/index.js +++ b/lib/index.js @@ -71,7 +71,19 @@ function optimizeJs (jsString, opts) { node.parentNode.callee === node } - if (isArgumentToFunctionCall(node) || isElementOfArrayArgumentToFunctionCall(node)) { + // tries to divine if this function is a webpack module wrapper. + // returns true iff node is an element of an array literal which is in turn + // an argument to a function call expression, and that function call + // expression is an IIFE. + function isProbablyWebpackModule (node) { + return isElementOfArrayArgumentToFunctionCall(node) && + node.parentNode && // array literal + node.parentNode.parentNode && // CallExpression + node.parentNode.parentNode.callee && // function that is being called + node.parentNode.parentNode.callee.type === 'FunctionExpression' + } + + if (isArgumentToFunctionCall(node) || isProbablyWebpackModule(node)) { // function passed in to another function, either as an argument, or as an element // of an array argument. these are almost _always_ executed, e.g. webpack bundles, // UMD bundles, Browserify bundles, Node-style errbacks, Promise then()s and catch()s, etc. diff --git a/test/cases/array-literal-in-function-params/input.js b/test/cases/array-literal-in-function-params/input.js new file mode 100644 index 0000000..ebf1cc6 --- /dev/null +++ b/test/cases/array-literal-in-function-params/input.js @@ -0,0 +1,5 @@ +foo([ + function(o,r,t){ + console.log("element 0"); + } +]); diff --git a/test/cases/array-literal-in-function-params/output.js b/test/cases/array-literal-in-function-params/output.js new file mode 100644 index 0000000..ebf1cc6 --- /dev/null +++ b/test/cases/array-literal-in-function-params/output.js @@ -0,0 +1,5 @@ +foo([ + function(o,r,t){ + console.log("element 0"); + } +]); From 3b81b45b860984b29840c3a80309fbc5e0e8984c Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Sun, 30 Oct 2016 14:38:54 -0700 Subject: [PATCH 4/5] Added optimization of browserify modules. --- lib/index.js | 38 +++++++++++++++++-- .../input.js | 9 +++++ .../output.js | 9 +++++ .../browserify-style-multi-element/input.js | 24 ++++++++++++ .../browserify-style-multi-element/output.js | 24 ++++++++++++ test/cases/browserify-style-non-iife/input.js | 10 +++++ .../cases/browserify-style-non-iife/output.js | 10 +++++ .../browserify-style-one-element/input.js | 12 ++++++ .../browserify-style-one-element/output.js | 12 ++++++ .../input.js | 14 +++++++ .../output.js | 14 +++++++ 11 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 test/cases/browserify-object-literal-in-global-scope/input.js create mode 100644 test/cases/browserify-object-literal-in-global-scope/output.js create mode 100644 test/cases/browserify-style-multi-element/input.js create mode 100644 test/cases/browserify-style-multi-element/output.js create mode 100644 test/cases/browserify-style-non-iife/input.js create mode 100644 test/cases/browserify-style-non-iife/output.js create mode 100644 test/cases/browserify-style-one-element/input.js create mode 100644 test/cases/browserify-style-one-element/output.js create mode 100644 test/cases/browserify-style-with-non-numeric-propname/input.js create mode 100644 test/cases/browserify-style-with-non-numeric-propname/output.js diff --git a/lib/index.js b/lib/index.js index 21b2e5c..d01e338 100644 --- a/lib/index.js +++ b/lib/index.js @@ -42,6 +42,10 @@ function optimizeJs (jsString, opts) { return false } + function isNumeric (str) { + return /^[0-9]+$/.test(str) + } + function isCallExpression (node) { return node && node.type === 'CallExpression' } @@ -57,14 +61,31 @@ function optimizeJs (jsString, opts) { node.parentNode.arguments.indexOf(node) !== -1 } + function isElementOfArray (node) { + return isArrayExpression(node.parentNode) && + node.parentNode.elements.indexOf(node) !== -1 + } + // returns true iff node is an element of an array literal which is in turn // an argument to a function call expression. function isElementOfArrayArgumentToFunctionCall (node) { - return isArrayExpression(node.parentNode) && - node.parentNode.elements.indexOf(node) !== -1 && + return isElementOfArray(node) && isArgumentToFunctionCall(node.parentNode) } + function isValueOfObjectLiteralWithNumericName (node) { + return node && + node.parentNode && + node.parentNode.type === 'ObjectExpression' && + node.parentNode.properties.some(property => { + return property.key && + property.key.type === 'Literal' && + property.key.raw && + isNumeric(property.key.raw) && + property.value === node + }) + } + // returns true iff node is an IIFE. function isIIFE (node) { return isCallExpression(node.parentNode) && @@ -83,7 +104,18 @@ function optimizeJs (jsString, opts) { node.parentNode.parentNode.callee.type === 'FunctionExpression' } - if (isArgumentToFunctionCall(node) || isProbablyWebpackModule(node)) { + function isProbablyBrowserifyModule (node) { + return isElementOfArray(node) && + isValueOfObjectLiteralWithNumericName(node.parentNode) && + isArgumentToFunctionCall(node.parentNode.parentNode) && + node.parentNode.parentNode.parentNode && // function call expression + node.parentNode.parentNode.parentNode.callee && // function that is being called + node.parentNode.parentNode.parentNode.callee.type === 'FunctionExpression' + } + + if (isArgumentToFunctionCall(node) || + isProbablyWebpackModule(node) || + isProbablyBrowserifyModule(node)) { // function passed in to another function, either as an argument, or as an element // of an array argument. these are almost _always_ executed, e.g. webpack bundles, // UMD bundles, Browserify bundles, Node-style errbacks, Promise then()s and catch()s, etc. diff --git a/test/cases/browserify-object-literal-in-global-scope/input.js b/test/cases/browserify-object-literal-in-global-scope/input.js new file mode 100644 index 0000000..d623627 --- /dev/null +++ b/test/cases/browserify-object-literal-in-global-scope/input.js @@ -0,0 +1,9 @@ +// a browserify-style object literal that isn't a parameter shouldn't be optimized. +var a = { + 1:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] +}; diff --git a/test/cases/browserify-object-literal-in-global-scope/output.js b/test/cases/browserify-object-literal-in-global-scope/output.js new file mode 100644 index 0000000..d623627 --- /dev/null +++ b/test/cases/browserify-object-literal-in-global-scope/output.js @@ -0,0 +1,9 @@ +// a browserify-style object literal that isn't a parameter shouldn't be optimized. +var a = { + 1:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] +}; diff --git a/test/cases/browserify-style-multi-element/input.js b/test/cases/browserify-style-multi-element/input.js new file mode 100644 index 0000000..32f8e47 --- /dev/null +++ b/test/cases/browserify-style-multi-element/input.js @@ -0,0 +1,24 @@ +!function(o){ + return o[0](); +}( + { + 1:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ], + 2:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ], + 3:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] + } +); diff --git a/test/cases/browserify-style-multi-element/output.js b/test/cases/browserify-style-multi-element/output.js new file mode 100644 index 0000000..63cdf13 --- /dev/null +++ b/test/cases/browserify-style-multi-element/output.js @@ -0,0 +1,24 @@ +!(function(o){ + return o[0](); +})( + { + 1:[ + (function(o,r,t){ + console.log("browserify style!"); + }), + {} + ], + 2:[ + (function(o,r,t){ + console.log("browserify style!"); + }), + {} + ], + 3:[ + (function(o,r,t){ + console.log("browserify style!"); + }), + {} + ] + } +); diff --git a/test/cases/browserify-style-non-iife/input.js b/test/cases/browserify-style-non-iife/input.js new file mode 100644 index 0000000..29227ed --- /dev/null +++ b/test/cases/browserify-style-non-iife/input.js @@ -0,0 +1,10 @@ +foo( + { + 1:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] + } +); diff --git a/test/cases/browserify-style-non-iife/output.js b/test/cases/browserify-style-non-iife/output.js new file mode 100644 index 0000000..29227ed --- /dev/null +++ b/test/cases/browserify-style-non-iife/output.js @@ -0,0 +1,10 @@ +foo( + { + 1:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] + } +); diff --git a/test/cases/browserify-style-one-element/input.js b/test/cases/browserify-style-one-element/input.js new file mode 100644 index 0000000..71f9e66 --- /dev/null +++ b/test/cases/browserify-style-one-element/input.js @@ -0,0 +1,12 @@ +!function(o){ + return o[0](); +}( + { + 1:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] + } +); diff --git a/test/cases/browserify-style-one-element/output.js b/test/cases/browserify-style-one-element/output.js new file mode 100644 index 0000000..6741ca5 --- /dev/null +++ b/test/cases/browserify-style-one-element/output.js @@ -0,0 +1,12 @@ +!(function(o){ + return o[0](); +})( + { + 1:[ + (function(o,r,t){ + console.log("browserify style!"); + }), + {} + ] + } +); diff --git a/test/cases/browserify-style-with-non-numeric-propname/input.js b/test/cases/browserify-style-with-non-numeric-propname/input.js new file mode 100644 index 0000000..8feba69 --- /dev/null +++ b/test/cases/browserify-style-with-non-numeric-propname/input.js @@ -0,0 +1,14 @@ +// a browserify-style function call that has non-numeric object indices shouldn't +// be optimized. +!function(o){ + return o[0](); +}( + { + a:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] + } +); diff --git a/test/cases/browserify-style-with-non-numeric-propname/output.js b/test/cases/browserify-style-with-non-numeric-propname/output.js new file mode 100644 index 0000000..6a4954c --- /dev/null +++ b/test/cases/browserify-style-with-non-numeric-propname/output.js @@ -0,0 +1,14 @@ +// a browserify-style function call that has non-numeric object indices shouldn't +// be optimized. +!(function(o){ + return o[0](); +})( + { + a:[ + function(o,r,t){ + console.log("browserify style!"); + }, + {} + ] + } +); From b07c712cc883013e5f6162f17ae5d1dfb5001153 Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Sun, 30 Oct 2016 14:57:28 -0700 Subject: [PATCH 5/5] Refactor of browserify and webpack logic. --- lib/index.js | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/index.js b/lib/index.js index d01e338..cf2f4bf 100644 --- a/lib/index.js +++ b/lib/index.js @@ -66,13 +66,6 @@ function optimizeJs (jsString, opts) { node.parentNode.elements.indexOf(node) !== -1 } - // returns true iff node is an element of an array literal which is in turn - // an argument to a function call expression. - function isElementOfArrayArgumentToFunctionCall (node) { - return isElementOfArray(node) && - isArgumentToFunctionCall(node.parentNode) - } - function isValueOfObjectLiteralWithNumericName (node) { return node && node.parentNode && @@ -88,29 +81,35 @@ function optimizeJs (jsString, opts) { // returns true iff node is an IIFE. function isIIFE (node) { - return isCallExpression(node.parentNode) && + return node && + node.type === 'FunctionExpression' && + isCallExpression(node.parentNode) && node.parentNode.callee === node } + // returns true iff this is an IIFE call expression + function isIIFECall (node) { + return node && + isCallExpression(node) && + node.callee && + node.callee.type === 'FunctionExpression' + } + // tries to divine if this function is a webpack module wrapper. // returns true iff node is an element of an array literal which is in turn // an argument to a function call expression, and that function call // expression is an IIFE. function isProbablyWebpackModule (node) { - return isElementOfArrayArgumentToFunctionCall(node) && - node.parentNode && // array literal - node.parentNode.parentNode && // CallExpression - node.parentNode.parentNode.callee && // function that is being called - node.parentNode.parentNode.callee.type === 'FunctionExpression' + return isElementOfArray(node) && + isArgumentToFunctionCall(node.parentNode) && + isIIFECall(node.parentNode.parentNode) } function isProbablyBrowserifyModule (node) { return isElementOfArray(node) && isValueOfObjectLiteralWithNumericName(node.parentNode) && isArgumentToFunctionCall(node.parentNode.parentNode) && - node.parentNode.parentNode.parentNode && // function call expression - node.parentNode.parentNode.parentNode.callee && // function that is being called - node.parentNode.parentNode.parentNode.callee.type === 'FunctionExpression' + isIIFECall(node.parentNode.parentNode.parentNode) } if (isArgumentToFunctionCall(node) ||