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

Spec compatibility for template literals. #5791

Merged
merged 16 commits into from Jun 5, 2017
Merged

Conversation

@yavorsky
Copy link
Member

yavorsky commented May 30, 2017

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

Without spec option there is spec compatibility problem:
+ always passes "default" hint, but not "string".

For ex:

const obj = {
  [Symbol.toPrimitive](hint) {
    if (hint == 'number') {
      return 1;
    }
    if (hint == 'string') {
      return 'hello';
    }
    return true;
  }
};

`${obj}` // returns 'hello' as expected.
// After babel transform w/o spec option:
"" + obj; // returns 'true';

It was resolved and described under babel/babel#1065, but the solution isn't the best and could be optimized.

For now, with spec option we just wrap expressions with String:
"a" + String(1) + "b" + String("c")

But for ex, if one of the template literal expressions is a Symbol() it won't throw.

`${Symbol()}` // Will throw TypeError: Cannot convert a Symbol value to a string
"" + String(Symbol()) // Won't throw.

String constructor special-cases symbols and return description string instead of throwing.
So, proposed solution is to use String.prototype.concat which handle this cases according to the spec:

in

`a${1}${"b"}${"c"}`

out

`"a".concat(1, "b", "c")`

thanks @shvaikalesh for pointing me to this issue and help!

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented May 30, 2017

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

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 30, 2017

Codecov Report

Merging #5791 into 7.0 will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #5791      +/-   ##
==========================================
- Coverage   84.81%   84.81%   -0.01%     
==========================================
  Files         282      282              
  Lines        9874     9887      +13     
  Branches     2776     2780       +4     
==========================================
+ Hits         8375     8386      +11     
  Misses        990      990              
- Partials      509      511       +2
Impacted Files Coverage Δ
...in-transform-es2015-template-literals/src/index.js 100% <100%> (ø) ⬆️
packages/babel-helper-call-delegate/src/index.js 64% <0%> (-4%) ⬇️
packages/babel-traverse/src/path/modification.js 72.11% <0%> (-0.97%) ⬇️
packages/babel-traverse/src/visitors.js 85.71% <0%> (-0.96%) ⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.12% <0%> (-0.86%) ⬇️
packages/babel-traverse/src/path/introspection.js 45.23% <0%> (-0.6%) ⬇️
packages/babel-traverse/src/path/context.js 86.2% <0%> (+0.86%) ⬆️
packages/babel-helper-fixtures/src/index.js 75.67% <0%> (+4.05%) ⬆️

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 5fbe8ed...27942ab. Read the comment docs.

Copy link
Member

jridgewell left a comment

Overall, this looks good.

nodes.unshift(t.stringLiteral(""));
}
let root = nodes.shift();

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 30, 2017

Member

I just got rid of this #shift 😛. Let's do:

let root = nodes[0];
if (spec && nodes.length > 1) {
  root = t.callExpression(t.memberExpression(target, t.identifier("concat")), nodes.slice(1));
} else {
  for (;;) {
    // ...
  }

Whether you use the helper function is up to you.

This comment has been minimized.

Copy link
@yavorsky

yavorsky May 30, 2017

Author Member

Yeah, I just saw you had refactored this previously.
I'm ok with a bit of immutability 👍

@bakkot

This comment has been minimized.

Copy link
Contributor

bakkot commented May 30, 2017

This still isn't quite right, if we're being really pedantic:

`
${
  console.log(1),
  {
    [Symbol.toPrimitive](){
      console.log(2);
      return 'foo';
    }
  }
}
${
  console.log(3),
  {
    [Symbol.toPrimitive](){
      console.log(4);
      return 'bar';
    }
  }
}
`

Per spec, this will log 1 2 3 4. After this transform, if I'm reading it right, the resulting program will log 1 3 2 4.

You could use a series of concat calls instead, which would have the right semantics. Alternatively you could define a custom ToString helper, which did something like s => typeof s === 'symbol' ? '' + s : String(s).

@yavorsky

This comment has been minimized.

Copy link
Member Author

yavorsky commented May 30, 2017

@bakkot
Good catch! "".concat(a).concat(b) is a solution. And according to the jsPerf there isn't perf impact.

Also, what do you think about wrapping expressions into arrays instead of String.
[a] + [b] + [Symbol()]

@bakkot

This comment has been minimized.

Copy link
Contributor

bakkot commented May 31, 2017

Strongly prefer concat to wrapping in arrays. It's much clearer what the intent is.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented May 31, 2017

Hmm. Purely to minimize output, we might be able to group strings into the last concat call:

const calls = nodes.reduce((acc, node) => {
  if (t.isStringLiteral(node)) {
    last(acc).push(node);
  } else {
    nodes.push([node]);
  }
});

for (let i = 0; i < calls.length; i++) {
  root = buildConcat(root, calls[i]);
}
@yavorsky

This comment has been minimized.

Copy link
Member Author

yavorsky commented Jun 1, 2017

Ok, I've updated some stuff to ensure the toPrimitive calls order.

@jridgewell I took your proposition as a basis and enhanced it to fill arguments of the last concat call unless we met the next node which not a string.

So, it looks like: "a".concat("b", c, "d", "e").concat(f).concat(5,"g");

Also, it's good to add a test case to check toPrimitive calls order. The problem is that Symbol.toPrimitive isn't supported by node 4.

Copy link
Member

jridgewell left a comment

Please add @bakkot's example as an exec.js, and include yours as an actual.js/expected.js.

@@ -1,7 +1,28 @@
export default function ({ types: t }) {

function buildConcatCallExressions(items) {
return items.reduce(function(left, right) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jun 1, 2017

Member

To avoid the _withIdentifier, can't we rewrite to:

let first = true;
return items.reduce((left, right) => {
  // Use isLiteral instead, 3 and null, and etc.
  let canInsert = t.isLiteral(right);
  if (!canInsert && first) {
    canInsert = true;
    first = false;
  }
  if (canInsert) {
    left.arguments.push(right);
    return left;
  }
  return t.callExpression(
    t.memberExpression(left, t.identifier('concat')),
    [right]
  );
});
@yavorsky

This comment has been minimized.

Copy link
Member Author

yavorsky commented Jun 2, 2017

@jridgewell Updated.
All tests except one passed. As I mentioned above, Symbol.toPrimitive isn't supported in node 4.
I could add something like if (!Symbol.toPrimitive) {calls.push(2)}, but not sure it's the perfect idea.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Jun 2, 2017

We can use minNodeVersion to restrict that test.

@bakkot
bakkot approved these changes Jun 2, 2017
canBeInserted = true;
avail = false;
}
if (t.isCallExpression(left) && canBeInserted) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Jun 2, 2017

Member

Nit: we can flip the LogicalExpression to save a function call in some cases.

@@ -75,7 +75,7 @@ In loose mode, tagged template literal objects aren't frozen.

`boolean`, defaults to `false`.

This option wraps all template literal expressions with `String`. See [babel/babel#1065](https://github.com/babel/babel/issues/1065) for more info.

This comment has been minimized.

Copy link
@hzoo

hzoo Jun 3, 2017

Member

Why did we remove the link? We should add a reason for why this is necessary or a good idea so people understand why they would use this option (if it's just old then we can add a new sentence here)

This comment has been minimized.

Copy link
@yavorsky

yavorsky Jun 3, 2017

Author Member

@hzoo I think we can add a link to this PR in README. Also going to add a few sentences about spec option and why we are using String.prototype.concat.

@hzoo
hzoo approved these changes Jun 3, 2017
Copy link
Member

hzoo left a comment

nice work! Just wondering about the docs for this in the readme (why use this, an example) - maybe loose needs that too

@hzoo hzoo merged commit c4fd05c into babel:7.0 Jun 5, 2017
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Jun 5, 2017

Awesome work @yavorsky 🎉

gdh1995 added a commit to gdh1995/viewerjs that referenced this pull request Jan 1, 2019
The behavior before is `"".concat(part1, part2, ...)`,
which is useless and makes the minified file larger (about 600 bytes).

The benefit of `concat` is just supporting `Symbol.toPrimitive`.
Therefore, we should replace it with simpler `+`.

Doc:
* https://babeljs.io/docs/en/babel-plugin-transform-template-literals
* babel/babel#5791
@denisw denisw mentioned this pull request Jan 11, 2019
gdh1995 added a commit to gdh1995/viewerjs that referenced this pull request Jun 4, 2019
The behavior before is `"".concat(part1, part2, ...)`,
which is useless and makes the minified file larger (about 600 bytes).

The benefit of `concat` is just supporting `Symbol.toPrimitive`.
Therefore, we should replace it with simpler `+`.

Doc:
* https://babeljs.io/docs/en/babel-plugin-transform-template-literals
* babel/babel#5791
gdh1995 added a commit to gdh1995/viewerjs that referenced this pull request Jul 30, 2019
The behavior before is `"".concat(part1, part2, ...)`,
which is useless and makes the minified file larger (about 600 bytes).

The benefit of `concat` is just supporting `Symbol.toPrimitive`.
Therefore, we should replace it with simpler `+`.

Doc:
* https://babeljs.io/docs/en/babel-plugin-transform-template-literals
* babel/babel#5791
gdh1995 added a commit to gdh1995/viewerjs that referenced this pull request Aug 25, 2019
The behavior before is `"".concat(part1, part2, ...)`,
which is useless and makes the minified file larger (about 600 bytes).

The benefit of `concat` is just supporting `Symbol.toPrimitive`.
Therefore, we should replace it with simpler `+`.

Doc:
* https://babeljs.io/docs/en/babel-plugin-transform-template-literals
* babel/babel#5791
@lock lock bot added the outdated label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.