-
Notifications
You must be signed in to change notification settings - Fork 103
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
Webpack-specific fix for issue #7 #33
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a9a37e3
Fix for #7: Not as optimized for Webpack as it could be. Adds ability…
aickin 4895d38
Added a few negative case tests in response to code review.
aickin aa9efd3
Modified the array literal logic to very specifically track with webp…
aickin 6904e09
Merged upstream master and fixed test failures that resulted from mov…
aickin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,15 +11,25 @@ function optimizeJs (jsString, opts) { | |
|
||
walk(ast, { | ||
enter: function (node, parent) { | ||
// assuming this node is an argument to a function, return true if it itself | ||
// is already padded with parentheses | ||
// estree-walker does not automatically add a parent node pointer to nodes, | ||
// which we need for some of our context checks below. | ||
// normally I would just write "node.parentNode = parent" here, but that makes | ||
// estree-walker think that parentNode is a child node of the node, which leads to | ||
// infinite loops as it walks a circular tree. if we make parent a function, though, | ||
// estree-walker does not follow the link. | ||
node.parent = function () { | ||
return parent | ||
} | ||
// 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 = parent.arguments.indexOf(node) | ||
var parentArray = node.parent().arguments ? node.parent().arguments : node.parent().elements | ||
var idx = parentArray.indexOf(node) | ||
if (idx === 0) { // first arg | ||
if (prePreChar === '(' && preChar === '(' && postChar === ')') { // already padded | ||
return true | ||
} | ||
} else if (idx === parent.arguments.length - 1) { // last arg | ||
} else if (idx === parentArray.length - 1) { // last arg | ||
if (preChar === '(' && postChar === ')' && postPostChar === ')') { // already padded | ||
return true | ||
} | ||
|
@@ -31,30 +41,66 @@ function optimizeJs (jsString, opts) { | |
return false | ||
} | ||
|
||
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.parent()) && | ||
node.parent().arguments.length && | ||
node.parent().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.parent()) && | ||
node.parent().elements.indexOf(node) !== -1 && | ||
isArgumentToFunctionCall(node.parent()) | ||
} | ||
|
||
// returns true iff node is an IIFE. | ||
function isIIFE (node) { | ||
return isCallExpression(node.parent()) && | ||
node.parent().callee === 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.parent() && // array literal | ||
node.parent().parent() && // CallExpression | ||
node.parent().parent().callee && // function that is being called | ||
node.parent().parent().callee.type === 'FunctionExpression' | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. love the name of this function! 😂 |
||
|
||
if (node.type === 'FunctionExpression') { | ||
var prePreChar = jsString.charAt(node.start - 2) | ||
var preChar = jsString.charAt(node.start - 1) | ||
var postChar = jsString.charAt(node.end) | ||
var postPostChar = jsString.charAt(node.end + 1) | ||
|
||
if (parent && parent.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 (parent.arguments && parent.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 (parent.callee === node) { | ||
// this function is getting immediately invoked, e.g. function(){}() | ||
if (preChar !== '(') { | ||
magicString.insertLeft(node.start, '(') | ||
.insertRight(node.end, ')') | ||
} | ||
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. | ||
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, ')') | ||
} | ||
} | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
function b() { | ||
var a = [ | ||
!function() {return 1;}() | ||
]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
function b() { | ||
var a = [ | ||
!(function() {return 1;})() | ||
]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
var a = [ | ||
!function() {return 1;}() | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
var a = [ | ||
!(function() {return 1;})() | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
foo([ | ||
function(o,r,t){ | ||
console.log("element 0"); | ||
} | ||
]); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
foo([ | ||
function(o,r,t){ | ||
console.log("element 0"); | ||
} | ||
]); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
function b() { | ||
var a = [ | ||
function() {return 1;} | ||
]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
function b() { | ||
var a = [ | ||
function() {return 1;} | ||
]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
var a = [ | ||
function() {return 1;} | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
var a = [ | ||
function() {return 1;} | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"); | ||
}]); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"); | ||
})]); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
!function(o){ | ||
return o[0](); | ||
}([function(o,r,t){ | ||
console.log("webpack style!"); | ||
}]); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
!(function(o){ | ||
return o[0](); | ||
})([(function(o,r,t){ | ||
console.log("webpack style!"); | ||
})]); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree this is a weird hack, but at least you marked it as such and added a lot of explanation :)