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

Shorten assignments to itself #230

Merged
merged 16 commits into from
Nov 13, 2016
Original file line number Diff line number Diff line change
Expand Up @@ -2136,4 +2136,57 @@ describe("simplify-plugin", () => {
const expected = source;
expect(transform(source)).toBe(expected);
});

it("should transform assignments to the same identifier", () => {

const actual = [
'x = x + 1;',
'x = x - 1;',
'x = x * 1;',
'x = x % 1;',
'x = x << 1;',
'x = x >> 1;',
'x = x >>> 1;',
'x = x & 1;',
'x = x | 1;',
'x = x ^ 1;',
'x = x / 1;',
'x = x ** 1;',
'foo = foo + bar;',
'foo = foo * function(){};',
'foo += 123;',
'foo = 1 + foo;',
'x = x = x + 1;',
'foo = foo + bar + baz;',
'window.foo = window.foo + 1;',
'window.foo.bar = window.foo.baz + 1;',
];

const expected = [
'x++;',
'x--;',
'x *= 1;',
'x %= 1;',
'x <<= 1;',
'x >>= 1;',
'x >>>= 1;',
'x &= 1;',
'x |= 1;',
'x ^= 1;',
'x /= 1;',
'x **= 1;',
'foo += bar;',
'foo *= function () {};',
'foo += 123;',
'foo = 1 + foo;',
'x = x++;',
'foo = foo + bar + baz;',
'window.foo = window.foo + 1;',
'window.foo.bar = window.foo.baz + 1;',
];

actual.forEach((test, index) => {
expect(transform(test)).toBe(expected[index]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect(actual.map((s) => transform(s))).toEqual(expected);

here you compare two arrays and even if one fails in the list, the others still run. But when you do forEach when one of them fails, all else that follows won't be run at all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're thinking to wrap them in try/catch?

});
});
});
54 changes: 54 additions & 0 deletions packages/babel-plugin-minify-simplify/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@ module.exports = ({ types: t }) => {
const or = (a, b) => t.logicalExpression("||", a, b);
const and = (a, b) => t.logicalExpression("&&", a, b);

const operators = new Set([
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to consider splitting plugins into multiple modules. This kind of mixing of helpers and supplemental stuff is not great ^. I think we could even strive for 1 type-of-transform per file.

'+', '-', '*', '%',
'<<', '>>', '>>>',
'&', '|', '^', '/',
'**'
]);

const updateOperators = new Set([
'+', '-'
]);

return {
name: "minify-simplify",
visitor: {
Expand Down Expand Up @@ -124,6 +135,49 @@ module.exports = ({ types: t }) => {
],
},

AssignmentExpression(path) {

const right = path.get('right');
const left = path.get('left');

const canShorten = (
right.type === 'BinaryExpression' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right.isBinaryExpression()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks!

operators.has(right.node.operator) &&
left.node.name === right.node.left.name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requires isIdentifier check. if left.node and right.node.left are not identifiers, it can end up in undefined === undefined check and canShorten will be true - eg: [a,b] = foo() + 1 - canShorten will be true for this.

);

if (!canShorten) return;

const canBeUpdateExpression = (
right.node.right.type === 'NumericLiteral' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right.get("right").isNumericLiteral() or t.isNumericLiteral(right.node.right)

right.node.right.value === 1 &&
updateOperators.has(right.node.operator));

if (left.node.name === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left.isMemberExpression() is better IMO, rather than depending on left not being an identifier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if it's always going to be member expression. This felt like a broader/safer check. But sure, probably ok to change it.

// TODO: transform MemberExpressions as well
// e.g. foo.bar = foo.bar + 123;
return;
}

if (canBeUpdateExpression) {

const newExpression = t.updateExpression(
right.node.operator + right.node.operator,
t.identifier(left.node.name));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably needs to handle member expression here ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we can make a generic utility for handling both Identifier/MemberExpression?


path.replaceWith(newExpression);
}
else {

const newExpression = t.assignmentExpression(
right.node.operator + '=',
t.identifier(left.node.name),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle memberExpressions

t.clone(right.node.right));

path.replaceWith(newExpression);
}
},

ConditionalExpression: {
enter: [
// !foo ? 'foo' : 'bar' -> foo ? 'bar' : 'foo'
Expand Down