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

refactor mark declaration, add __PURE__ comments (#286) #309

Merged
merged 5 commits into from Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 30 additions & 26 deletions packages/regenerator-transform/src/visit.js
Expand Up @@ -153,6 +153,7 @@ exports.visitor = {

if (wasGeneratorFunction && t.isExpression(node)) {
util.replaceWithOrRemove(path, t.callExpression(util.runtimeProperty("mark"), [node]))
path.addComment("leading", "#__PURE__");
}

// Generators are processed in 'exit' handlers so that regenerator only has to run on
Expand Down Expand Up @@ -188,18 +189,34 @@ function getOuterFnExpr(funPath) {
}

let markDecl = getRuntimeMarkDecl(pp);
let markedArray = markDecl.declarations[0].id;
let funDeclIdArray = markDecl.declarations[0].init.callee.object;
t.assertArrayExpression(funDeclIdArray);

let index = funDeclIdArray.elements.length;
funDeclIdArray.elements.push(node.id);

return t.memberExpression(
markedArray,
t.numericLiteral(index),
true
);
let declarations = markDecl.declarations;
let index = declarations.length;

// this can change from pass to pass if something else is altering our body
// so we have to look it up every time :/
let declarationsIndex = pp.node.body.findIndex((p) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could gather those declarations in context instead of inserting them right away and insert them all at once in program's exit visitor?

aint sure if thats better, just an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to look into this, it's probably best to not have to traverse the tree downward every time. i'm new to babel/ast stuff so it may take a bit to sort out.

Copy link
Contributor

Choose a reason for hiding this comment

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

im not a babel/ast expert either :P so i dont know what best practices are

just had such an idea how it should be possible to address the issue from the comment

Copy link
Collaborator

Choose a reason for hiding this comment

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

I pushed another commit that contains some improvements for this logic.

return t.isVariableDeclaration(p) && p.declarations === declarations;
});
assert(!isNaN(declarationsIndex));

// get a new unique id for our marked variable
let markedId = pp.scope.generateUidIdentifier("marked");

// now push our new declaration into our list
declarations.push(t.variableDeclarator(
markedId,
t.callExpression(util.runtimeProperty("mark"), [node.id])
));

// assemble the path to our mark declarations
let bodyPathName = 'body.' + String(declarationsIndex) + '.declarations.' + String(index) + '.init';
let bodyPath = pp.get(bodyPathName);

// mark our call to util.runtimeProperty("mark") as pure to enable tree-shaking
bodyPath.addComment("leading", "#__PURE__");

// and return our new id
return markedId;
}

return node.id;
Expand All @@ -214,20 +231,7 @@ function getRuntimeMarkDecl(blockPath) {
return info.decl;
}

info.decl = t.variableDeclaration("var", [
t.variableDeclarator(
blockPath.scope.generateUidIdentifier("marked"),
t.callExpression(
t.memberExpression(
t.arrayExpression([]),
t.identifier("map"),
false
),
[util.runtimeProperty("mark")]
)
)
]);

info.decl = t.variableDeclaration("var", [])
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks odd, should it stay here in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's basically the same functionality as before which was to memoize the place the declarations were to go - i guess since it's so much smaller now it could go in the caller insetad?

blockPath.unshiftContainer("body", info.decl);

return info.decl;
Expand Down
54 changes: 54 additions & 0 deletions test/tests.transform.js
Expand Up @@ -47,3 +47,57 @@ describe("_blockHoist nodes", function() {
assert.deepEqual(names, ["hoistMe", "doNotHoistMe", "oyez"]);
});
});

describe("marked", function() {
it("should work with a single function", function() {
var ast = recast.parse('function* foo(){};', {
parser: require("babylon")
});
const code = recast.print(transform(ast)).code;
assert.notStrictEqual(
code.indexOf(
'var _marked = regeneratorRuntime.mark(foo);'
), -1
);
});
it("should work with multiple functions", function() {
var ast = recast.parse([
'function* foo(){};',
'function* bar() {};'
].join("\n"), {
parser: require("babylon")
});
const code = recast.print(transform(ast)).code;
assert.notStrictEqual(
code.indexOf(
'var _marked = regeneratorRuntime.mark(foo), _marked2 = regeneratorRuntime.mark(bar);'
), -1
);
});
});

// TODO - how do i get recast to return comments?
Copy link
Contributor

Choose a reason for hiding this comment

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

i aint sure how what u are trying to achieve here exactly, ive managed to access comments like this:

recast.parse('var _marked = /*#__PURE__*/regeneratorRuntime.mark(test)').program.body[0].declarations[0].init.comments

maybe that helps you somehow

describe("pure", function() {
xit("should work with a function expression", function() {
var ast = recast.parse('var a = function* foo(){};', {
parser: require("babylon")
});
const code = recast.print(transform(ast)).code;
assert.notStrictEqual(
code.indexOf(
'var a = /*#__PURE__*/regeneratorRuntime.mark(function foo() {'
), -1
);
});
xit("should work with a function declaration", function() {
var ast = recast.parse('function* foo(){};', {
parser: require("babylon")
});
const code = recast.print(transform(ast)).code;
assert.notStrictEqual(
code.indexOf(
'var a = /*#__PURE__*/Object.defineProperty( /*#__PURE__*/regeneratorRuntime.mark'
Copy link
Contributor

Choose a reason for hiding this comment

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

why the Object.defineProperty call here? didnt we ditch it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, copy pasta. will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just updated all the tests, should be clearer now.

), -1
);
});
});