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

constant-folding: Fix some literal types used in .join(), closes #612 #613

Merged
merged 4 commits into from
Aug 15, 2017

Conversation

goto-bus-stop
Copy link
Contributor

The .join() inlining function was checking for isLiteral() on each array element, and then using the element's .value. Some literals don't have .values though: null literals, regexp literals and template literals.

This patch fixes .join()ing regexp literals and bails out of joining template literals entirely.

Null literals already did the right thing despite not having a .value, because .join() appears to treat undefined and null the same.

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Jul 2, 2017

Lint is failing, but if I npm run fix some files that are not relevant to this PR get changed too. Should I commit all of that or just the fixes to the files I changed?

e; I fixed just the lines i changed, looks like that did it ✌️

@j-f1
Copy link
Contributor

j-f1 commented Jul 2, 2017

@goto-bus-stop What about people using template strings like `foo`? You could check if templateLiteral.expressions.length === 0, which would indicate that it’s an ordinary template literal.

@goto-bus-stop
Copy link
Contributor Author

Ah yes, that's quite simple. Thanks for the suggestion!

}
return el.value;
if (t.isNullLiteral(el)) {
Copy link
Member

@vigneshshanmugam vigneshshanmugam Jul 2, 2017

Choose a reason for hiding this comment

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

IMO, we can do path.evaluate(el) and deopt.

If its gonna be killing performance which in most cases should not be a problem since the path would already be cached during the traversal phase.. Thoughts?

Copy link
Contributor Author

@goto-bus-stop goto-bus-stop Jul 2, 2017

Choose a reason for hiding this comment

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

You mean doing evaluate instead of all the is* checks?

e; The only problem I can see is that there's no access to the path in these replacement functions, so we'd need to pass it in somehow…

return el.value;
}
if (t.isTemplateLiteral(el) && el.expressions.length === 0) {
return el.quasis[0].value.cooked;
Copy link
Member

Choose a reason for hiding this comment

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

@boopathi
Copy link
Member

Thanks! Looks good to me. Can you address the pending comments?

@goto-bus-stop
Copy link
Contributor Author

Added a commit to use path.evaluate(). Previously the NodePath was not available inside replacement functions, only the node itself. I changed it to pass the NodePath to replacement functions, and changed all existing replacement functions to use this.node instead of just this where necessary. It makes the diff quite noisy.

@boopathi
Copy link
Member

LGTM. Looks like formatting issues on CI. you can fix it with yarn format

@goto-bus-stop
Copy link
Contributor Author

Oops, should've checked lint before committing. Thanks for the heads-up

@vigneshshanmugam vigneshshanmugam merged commit b295f35 into babel:master Aug 15, 2017
@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: Bug Fix Pull Request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants