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

default to spec mode for template literal transform #6098

Merged
merged 5 commits into from
Aug 28, 2017

Conversation

noahlemen
Copy link
Member

Q                       A
Fixed Issues
Patch: Bug Fix?
Major: Breaking Change? 👌 (change to transform option)
Minor: New Feature?
Tests Added/Pass?
Spec Compliancy?
License MIT
Doc PR
Any Dependency Changes?

Similar to #6076, change transform-es2015-template-literals to default to spec mode, with loose optional

When this was being discussed in slack, there was also talk of this transform not throwing errors properly in some cases, but I didn't totally follow. If this could be further clarified, I'd be happy to try to implement that fix in this PR as well.

@noahlemen
Copy link
Member Author

transform-es2015-arrow-functions and babel-preset-es2015 still default to non-spec mode -- should these be updated?

if (!t.isStringLiteral(nodes[0]) && considerSecondNode) {
nodes.unshift(t.stringLiteral(""));
}
let root = nodes[0];

if (state.opts.spec) {
if (!state.opts.loose) {
Copy link
Member

Choose a reason for hiding this comment

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

Flip the if statements, please.

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 23, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4695/

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4690/

root = buildConcatCallExressions(nodes);
}
} else {
if (state.opts.loose) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@jridgewell let me know if i misunderstood what you meant by "flip" here!

Copy link
Member

Choose a reason for hiding this comment

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

That was it.

@noahlemen
Copy link
Member Author

^^^ oh man these integrated REPL builds are sliiiiiiick

for (let i = 1; i < nodes.length; i++) {
root = t.binaryExpression("+", root, nodes[i]);
}
} else {
if (nodes.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you also merge else { if ( -> else if (?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 great idea!

`boolean`, defaults to `false`.

This option combines all template literal expressions and quasis with `String.prototype.concat`. It will handle cases with `Symbol.toPrimitive` correctly and throw correctly if template literal expression is a `Symbol()`. See [babel/babel#5791](https://github.com/babel/babel/pull/5791).
When `false` or not set, combines all template literal expressions and quasis with `String.prototype.concat`. It will handle cases with `Symbol.toPrimitive` correctly and throw correctly if template literal expression is a `Symbol()`. See [babel/babel#5791](https://github.com/babel/babel/pull/5791).
Copy link
Member

@existentialism existentialism Aug 23, 2017

Choose a reason for hiding this comment

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

When false or not set, combines all template...

Maybe:

When false or not set, all template literal expressions and quasis are combined with String.prototype.concat.


**In**

```javascript
`foo${bar}baz${quux}${1}`;
```

**Out**
**Out (without `{"loose": true}`)**
Copy link
Member

@existentialism existentialism Aug 23, 2017

Choose a reason for hiding this comment

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

I think it might be better if we update the example above with the default behavior, and keep this In/Out for loose: true

@existentialism existentialism added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Aug 23, 2017

This option combines all template literal expressions and quasis with `String.prototype.concat`. It will handle cases with `Symbol.toPrimitive` correctly and throw correctly if template literal expression is a `Symbol()`. See [babel/babel#5791](https://github.com/babel/babel/pull/5791).
When `false` or not set, all template literal expressions and quasis are combined with `String.prototype.concat`. It will handle cases with `Symbol.toPrimitive` correctly and throw correctly if template literal expression is a `Symbol()`. See [babel/babel#5791](https://github.com/babel/babel/pull/5791).
Copy link
Member Author

@noahlemen noahlemen Aug 23, 2017

Choose a reason for hiding this comment

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

should "See babel/babel#5791" stay or be removed?

@hzoo hzoo merged commit 4080589 into babel:7.0 Aug 28, 2017
@@ -1,3 +0,0 @@
var foo = bar`wow\na${ 42 }b ${_.foobar()}`;
Copy link
Member

Choose a reason for hiding this comment

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

did we mean to delete this tests or are the covered already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@noahlemen noahlemen Aug 28, 2017

Choose a reason for hiding this comment

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

er, maybe that isn't the same? i might have been mistaken. thought there was some test duplication, but it can be hard to wrap your head around that while working with so many rennamed files 🙃

Copy link
Member Author

@noahlemen noahlemen Aug 28, 2017

Choose a reason for hiding this comment

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

i'll have a chance to inspect this a bit more later. will check and submit a PR re-adding these tests if they were mistakenly removed

Copy link
Member Author

Choose a reason for hiding this comment

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

submitted #6169

@hzoo
Copy link
Member

hzoo commented Sep 20, 2017

For documentation, this causes an issue with webpack if you are using dynamic imports with template strings because we turn import(`${a}`) into .concat which webpack doesn't handle.

Issue: webpack/webpack#5674
PR: webpack/webpack#5679

We made it default so the workaround is to just you a normal string concat (don't use template literals) or use loose mode until webpack updates.

{
    presets: [
      require('babel-preset-env'),
    ],
    plugins: [
      [
        require('babel-plugin-transform-es2015-template-literals'), { loose: true },
      ],
    ],
  };

or if you are using loose mode for everything:

{
    presets: [
      [require('babel-preset-env'), { loose: true }],
    ],
  };

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants