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

Add support for evaluating `String.raw` expressions #5681

Merged
merged 2 commits into from Jun 24, 2017

Conversation

Projects
None yet
5 participants
@josephfrazier
Contributor

josephfrazier commented May 1, 2017

Q A
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? yes
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets no
License MIT
Doc PR no
Dependency Changes no

This makes it possible for Babel to statically evaluate String.raw
expressions like this:

String.raw`\d`

to values like this:

"\\d"

I made the changes in two commits:

  1. Implement the new functionality
  2. Deduplicate copy/pasted code into a helper function
@mention-bot

This comment has been minimized.

mention-bot commented May 1, 2017

@josephfrazier, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @boopathi and @DrewML to be potential reviewers.

@codecov

This comment has been minimized.

codecov bot commented May 1, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (7.0@6850064). Click here to learn what that means.
The diff coverage is 83.33%.

Impacted file tree graph

@@          Coverage Diff           @@
##             7.0    #5681   +/-   ##
======================================
  Coverage       ?   84.66%           
======================================
  Files          ?      283           
  Lines          ?     9741           
  Branches       ?     2737           
======================================
  Hits           ?     8247           
  Misses         ?      979           
  Partials       ?      515
Impacted Files Coverage Δ
packages/babel-traverse/src/path/evaluation.js 73.65% <83.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6850064...af126df. Read the comment docs.

josephfrazier added a commit to josephfrazier/xregexp that referenced this pull request May 1, 2017

Dedupe BMP pattern
Once babel/babel#5681 is merged/released and
babel-plugin-transform-xregexp is upgraded, we can use `String.raw` for
`bmpPattern` instead of double-escaping.
@hzoo

hzoo approved these changes May 1, 2017

Awesome, nice work!

const expr = exprs[i++];
if (expr) str += String(evaluate(expr));
if (
object.isIdentifier() && object.node.name === "String" &&

This comment has been minimized.

@loganfsmyth

loganfsmyth May 1, 2017

Member

I think we'll have to check scoping here, since this would also evaluate

function fn(String) {
  return String.raw``;
}
fn({raw: (strs, ...args) => ""});

This comment has been minimized.

@josephfrazier

josephfrazier May 1, 2017

Contributor

Oops, I missed that. Just pushed up c525e4f with a fix.

This comment has been minimized.

@hzoo

hzoo May 1, 2017

Member

I guess this kind of check needs to happen a lot

This comment has been minimized.

@josephfrazier

josephfrazier May 1, 2017

Contributor

Yeah, I just copied it from here. Hope that's correct.

// add on cooked element
str += elem.value.cooked;
if (path.isTaggedTemplateExpression()) {

This comment has been minimized.

@loganfsmyth

loganfsmyth May 1, 2017

Member

This should check path.get("tag").isMemberExpression()

This comment has been minimized.

@josephfrazier

josephfrazier May 1, 2017

Contributor

Thanks, fixed in af126df. I'm curious, though, is this check made redundant by the checks here? I'm not very familiar with Babel internals yet.

This comment has been minimized.

@hzoo

hzoo May 1, 2017

Member

Yeah it's necessary since if it's an identifier then you would be doing const { node: { name } } = object; of undefined since const object = path.get("tag.object"); object is undefined in that case.

So not really an internals thing just AST

This comment has been minimized.

@josephfrazier

josephfrazier May 1, 2017

Contributor

Ah, of course. I was still thinking of the code before I added in the destructuring, and I wasn't sure how path.get behaves with non-existent paths. Thanks for the explanation.

josephfrazier added a commit to josephfrazier/babel that referenced this pull request May 1, 2017

josephfrazier added a commit to josephfrazier/babel that referenced this pull request May 1, 2017

@josephfrazier

This comment has been minimized.

Contributor

josephfrazier commented May 1, 2017

Thanks for the reviews, @hzoo and @loganfsmyth. I've made some changes in response to the inline comments. Let me know what you think!

@hzoo

hzoo approved these changes May 2, 2017

@josephfrazier

This comment has been minimized.

Contributor

josephfrazier commented May 16, 2017

@loganfsmyth, does this need further changes in order to land? Thanks!

josephfrazier added some commits Jun 20, 2017

Add support for evaluating `String.raw` expressions
* Dedupe evaluation code for template literal quasis

* Check scoping in `String.raw` evaluation
  This addresses #5681 (comment)

* Ensure that `tag` is a MemberExpression in `String.raw` evaluation
  This addresses #5681 (comment)

@hzoo hzoo merged commit a330cf2 into babel:7.0 Jun 24, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 85.32% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@josephfrazier josephfrazier deleted the josephfrazier:evaluate-String.raw branch Jun 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment