Implemented transform-inline-consecutive-adds plugin. #286
Conversation
This plugin inlines consecutive attribute definitions.
shinew
changed the title
|
This looks great! Couple things:
|
| seen = true; | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
var foo = {};
foo.a = function() {
var foo = 1;
}There was a problem hiding this comment.
doing
binding.referencePaths[i].isDescendant(right)should give you correct result and a cheaper one too I suppose.
|
we aren't optimizing this, right ? var a = {};
a.b = {};
a.b.x = 1;
a.b.y = 2;
a.c = 3; |
|
@boopathi not currently -- fixing a bug right now: e.g. var foo = {};
foo.a = 4;
foo.b = bar();
...
function bar() {
console.log(foo);
return 0;
}The bug is that the identifier of the created object (i.e. |
var foo = {};
foo.a = 4;
foo[bar()] = 5;
...
function bar() {
console.log(foo);
return 0;
}
|
|
Awesome, thanks! Let's just rename the plugin to "transform-consecutive-property-assignments" since "attributes" usually refer to something else in ES |
shinew
changed the title
| it("should collapse normal computed properties", () => { | ||
| const source = unpad(` | ||
| var foo = {}; | ||
| foo["a"] = 0; |
There was a problem hiding this comment.
These are not computed properties though. It's just a regular bracket-based property resolution. What I wanted us to do is transform to computed property, when there's an identifier var foo = {}; foo[x] = 1 to var foo = { [x]: 1 };.
However, on a second thought, this would mean we're turning potentially ES5 code to ES6 so it's a special kind of transformation. Let's ignore that for now and revisit later.
There was a problem hiding this comment.
I faced this in #242 as well. We could let the user decide based on options - outputLanguage: es2015|es2016|es2017|...:
| foo.add(3); | ||
| `); | ||
| const expected = unpad(` | ||
| var foo = new Set([1, 2, 3]); |
| it("should collapse statements for arrays", () => { | ||
| const source = unpad(` | ||
| var foo = [1, 2]; | ||
| foo.push(3, 4), foo.push(5); |
There was a problem hiding this comment.
I wonder if we should bother with property-based assignments:
var foo = [];
foo[1] = 'blah';
foo[2] = 'blah';transforming to:
var foo=[,"blah","blah"];
Closure Compiler does this, btw — https://closure-compiler.appspot.com/home#code%3Dvar%2520foo%2520%253D%2520%255B%255D%253B%250Afoo%255B1%255D%2520%253D%2520'blah'%253B%250Afoo%255B2%255D%2520%253D%2520'blah'%253B
They also use smart heuristic (based on final length?) and transform this:
var foo = [];
foo[3] = 'blah';
foo[5] = 'blah';
foo[7] = 'blah';to:
var foo=[,,,"blah",,"blah",,"blah"];but not:
var foo = [];
foo[10] = 'blah';
| { | ||
| "name": "babel-plugin-transform-inline-consecutive-adds", | ||
| "version": "0.0.1", | ||
| "description": "This plugin inlines consecutive property definitions, array pushes, etc.", |
| if (statements.length > 0) { | ||
| const addons = exprs.map((e) => collapser.extractAddon(e)); | ||
| addons.forEach((addon) => collapser.addAddon(t, addon, init)); | ||
| statements.forEach((s) => s.remove()); |
There was a problem hiding this comment.
I really like the overall architecture here, where we have a nice modular way of defining extractable behavior for various data types — arrays, objects, sets, whatever else comes in the future.
But I did find some names a little confusing while reading it and trying to figure it out.
I would rename addons to assignments — extractAssignment, addAssignment — since addons is a little ambiguous and feels out of place.
I also found makeCheckExpression a little confusing. How about makeExpressionValidator (alternatively getExpressionValidator or make/getExpressionChecker) and checkExpressionType --> isExpressionTypeValid
|
@boopathi mind taking a look at the implementation details? |
|
Sure ... I'll take a look tomorrow. |
|
This should be added to preset. But I guess, this was forked before preset options was merged to master. So we can merge this and I can add it to preset separately or @shinew, mind adding it to preset with some option name ? |
| visitor: { | ||
| VariableDeclaration(varDecl) { | ||
| const topLevel = validateTopLevel(varDecl); | ||
| if (topLevel === undefined) { |
| "./array-property-collapser", | ||
| "./set-collapser" | ||
| ].map((x) => { | ||
| const Collapser = require(x); |
There was a problem hiding this comment.
use require("./object-collapser"). Do not use dynamic requires - for bundling purposes.
| @@ -1,26 +1,27 @@ | |||
| jest.autoMockOff(); | |||
|
|
|||
| const mocks = [ | |||
| "babel-plugin-inline-consecutive-adds", | |||
| } | ||
|
|
||
| function getIdAndFunctionReferences(name, parent) { | ||
| const binding = parent.scope.getBinding(name); |
There was a problem hiding this comment.
this is currently failing when used with other plugins
There was a problem hiding this comment.
plugins that create a new binding - like for of transformation in es2015 block-scoping, etc...
There was a problem hiding this comment.
So what's the todo here? When new scopes are created, they should make sure the bindings are resolved properly (leaves AST in consistent state).
There was a problem hiding this comment.
This is currently an issue with mangler and deadcode. Running it as a separate plugin and path.scope.crawl to recalculate all the bindings and references ensures it's partly fixing most of the issues.
I'm not yet sure if this is problem here. If there is an unregistered binding, bailing out as early as possible without breaking the code - without applying the optimisation is actually fine.
Just pointing it out so it is verified.
There was a problem hiding this comment.
I see -- added check for when binding == false.
| .reduce((a, b) => a + b, 0); // sum | ||
|
|
||
| return (numCommaAdded + sizeOfRvals < statementsLength); | ||
| } |
There was a problem hiding this comment.
rvals created on the fly (by other plugins in the same transform cycle) won't have a start and end ...
There was a problem hiding this comment.
Done.
Aside: I don't like the fact that we need to check for the existence of .start and .end every time -- those should be guaranteed by the babel framework (as in the plugin handbook). When the AST is modified, it should update metadata like this (and also scoping).
| if (!obj.isIdentifier() || obj.node.name !== objName) { | ||
| return false; | ||
| } | ||
| if (!prop.isIdentifier() && checkReference(prop)) { |
There was a problem hiding this comment.
you should also check if this is a computed property ...
function foo() {
var a = {};
a[global] = bar; // shouldn't transform to { global: bar }
}There was a problem hiding this comment.
Ah. I should've added this comment in the place where you add it to the list of properties
There was a problem hiding this comment.
How would we know if it's a computed property?
| var foo = {}; | ||
| foo[global] = 0; | ||
| `); | ||
| expect(transform(source)).toBe(source); |
There was a problem hiding this comment.
why not transform to
var foo = {
[global]: 0
};There was a problem hiding this comment.
But this might require an option now .. Can we add some meaningful option for now ?
There was a problem hiding this comment.
Let's leave this for later. Wouldn't want to cram everything into this PR.
|
@shinew could you check if our size benchmark reports any positive size differences with this transform? |
|
@shinew could you please also fix the tests and we should be ready to go |
|
@kangax I tried running the benchmark ( Will update the test suite for this plugin. |
|
Ok, based on the benchmark error, looks like we have an issue with recently implemented var lifting behavior /cc @vigneshshanmugam |
|
Published to NPM |
|
I squashed. Take a pull Squash after merge: git log --oneline
# xxxxxx - Merge Commit
# ensure log shows Merge commit as the first commit
git rebase -i HEAD~2
# since it is a merge commit, git will show all the commits that were merged
# usual rebase stuff (pick -> squash except first commit)
# save and
git push -f origin master
# I usually verify like this -
# Before starting rebase -
# ensure latest changes
git pull --rebase
# fork a branch from master
git checkout -b squash-1
# go back to master and do the squash
git checkout master && git rebase -i HEAD~2
# check if anything changed
git diff squash-1
# Important - check if the previous commits are NOT affected
# That they EXACTLY have the SAME commit hashes
# vim is pretty good, you can use any diff tool.
vim -d <(git log --oneline | head -n 30) <(git log --oneline squash-1 | head -n 30)
# ensure head -n XX is set accordingly
# branch-name in the second git log command is set correctly. |
This plugin inlines consecutive attribute definitions.