Make babel-template nicer in a bunch of ways by loganfsmyth · Pull Request #6492 · babel/babel

loganfsmyth · 2017-10-16T21:21:25Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?	Y, minimally for the common usecases though
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?

Breaking changes:

Numeric placeholders like "$0" and "$1" ... expect you to either pass an array with those indices, or an object with the full name as a property, rather than using positional arguments. e.g.
```
template("$0 + $1")([ t.numericLiteral(4), t.numericLiteral(5) ])
// OR
template("$0 + $1")({ $0: t.numericLiteral(4), $1: t.numericLiteral(5) })
```
where before you did
```
template("$0 + $1")(t.numericLiteral(4), t.numericLiteral(5))
```
(without the array brackets).
Auto-expanding an array of values into an array was removed as a feature, e.g.
```
template(`[THING]`)({thing: [t.identifier("foo"), t.identifier("bar")]});
```
will throw instead of producing [foo, bar].
Searching for placeholders is performed up front and cached, rather than running when template is instantiated. This means placeholder names need to be obvious up front. This breaks the following:
- Template placeholders that used characters other than /[_$A-Z0-9]/ will need to pass a custom placeholderPattern or rename the placeholder. There are no cases of this in Babel's core plugins.
- Templates that included identifiers matching that pattern, but not meant for replacement, will start throwing exceptions about a missing replacement value. The only case of this in Babel's core plugins was in the helpers, which don't have any placeholders anyway, so disabling pattern matching was the best fix.

New features:

Better performance, since the templates aren't traversed anymore at instantiation time. Instantiation just iterates over an existing list of placeholders to swap out.
Templates can pass in strings as replacements, which will automatically be wrapped into Identifier nodes, or into StringLiteral nodes if the placeholder was a string.
Templates created via tagged literals are cached, so you can do
```
var tpl = template`4 + 5`;
var ast = tpl();
```
will essentially just perform a clone operation, because the AST will only be parsed the first time the template is encountered.
More utilities are exposed for templates, so you can enforce better return types, e.g. template.expression("4"), template.statement("function foo(){}"), template.statements(..), template.program(""), and template.smart (the current default).
Templates that just want to instantiate an AST immediately can skip the intermediate function and associated new Error and stack processing by using .ast on the template function:
```
var ast = template.ast`foo;`
```

loganfsmyth · 2017-10-16T21:25:37Z

packages/babel-helper-module-transforms/src/index.js

-          NAMESPACE: t.identifier(data.name),
-          IMPORT_NAME: t.identifier(importName),
+        template`
+          Object.defineProperty(EXPORTS, "EXPORT_NAME", {


I could also opt to use template.ast on a lot of these and do

Object.defineProperty(${metadata.exportName}, "${exportName}", {

but I kind of felt like it hurt readability of some of the templates.

loganfsmyth · 2017-10-16T21:26:53Z

packages/babel-plugin-transform-es2015-modules-amd/src/index.js

              MODULE_NAME: moduleName,

              AMD_ARGUMENTS: t.arrayExpression(amdArgs),
-              COMMONJS_ARGUMENTS: commonjsArgs,


The nice thing about adding validation is that it catches all you're dumb mistakes :D

babel-bot · 2017-10-16T21:29:27Z

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

loganfsmyth · 2017-10-16T21:29:33Z

packages/babel-template/package.json

  "main": "lib/index.js",
  "dependencies": {
    "babel-code-frame": "7.0.0-beta.3",
-    "babel-traverse": "7.0.0-beta.3",


This severs the cyclic dependency that I filed. By making babel-template only depend on babel-types, we avoid risking cycles that we'd otherwise get if we split things out of babel-traverse but still wanted to use them as dependencies in babel-traverse.

hzoo · 2017-10-17T14:34:17Z

packages/babel-helper-module-transforms/src/index.js

-      ? buildNameListCheck({
-          EXPORTS_LIST: t.identifier(metadata.exportNameListName),
-        })
+      ? template`


is this also a template.statement or it doesn't matter?

The default is essentially autodetect statement or statements so I really didn't need to change the other cases either. Guess I just didn't do this one.

nicolo-ribaudo

I love this PR 🎉

nicolo-ribaudo · 2017-10-17T19:40:27Z

packages/babel-template/src/builder.js

+      // error.stack does not exists in IE <= 9
+      rootStack = error.stack
+        .split("\n")
+        .slice(3)


Can you add a comment which explains why 3? It looks too magic to me.

nicolo-ribaudo · 2017-10-17T19:46:45Z

packages/babel-template/src/formatters.js

+
+export type Formatter<T> = {
+  code: string => string,
+  validate: BabelNodeFile => void,


Where are BabelNode* types imported in this file? 🤔

Ideally we'd get Flowtype set up properly and have them live in babel-types, but for now they live in https://github.com/babel/babel/blob/master/lib/types.js

nicolo-ribaudo · 2017-10-17T19:48:51Z

packages/babel-template/src/formatters.js

+  validate: (ast: BabelNodeFile) => {
+    const { program } = ast;
+    if (program.directives && program.directives.length > 0) {
+      throw new Error("Unexpected directives found");


Does this mean that this code is invalid?

// I want an expression statement which contains a string literal const ast = template.statement`"string";`();

Yeah, you'd have to do

template.statement`("string");`();

if you wanted to get a string expression statement.

It's the same behavior we have right now, it's just that now it actually throws an error instead of silently discarding the string :P

I think we should wrap the code in { ... } so that my example is parsed as an expression statement. Otherwise it is inconsistent with template.statements, which wraps the input.

e.g.

template.statements`"string";`(); // [ ExpressionStatement {} ] template.statement`"string";`(); // Error! 💥

Or probably (better) we should use () => { /* input */ } as the wrapper in the statements formatter, so that directives are always parsed as directives.

They both throw at the moment, but I can certainly tweak this logic so strings are always interpreted as statements and not directives, if you think it'll be more ideal. I can see how the name of the functions might make that more surprising.

existentialism

❤️

nicolo-ribaudo · 2017-10-17T21:55:13Z

@loganfsmyth I just tested this PR locally: is this the expected behavior? Shouldn't template.statements return the BlockStatement's body?

const ast = template.statements.ast`foo; bar;`;
console.log(ast);

[ { type: 'BlockStatement',
    start: undefined,
    end: undefined,
    loc: undefined,
    body: [ [Object], [Object] ],
    directives: [] } ]

xtuc

@loganfsmyth is the best!

loganfsmyth · 2017-10-18T16:07:57Z

@nicolo-ribaudo Oh dang, you're right. I actually forgot that I'd wrapped statements in {} for parsing. I agree those should be consistent. I'm actually going to change that behavior so that smart, statement and statements all automatically treat strings as statements instead of directives, since that seems most consistent.

loganfsmyth · 2017-10-18T17:14:59Z

packages/babel-template/src/formatters.js

+  fn: (Array<BabelNodeStatement>) => T,
+): Formatter<T> {
+  return {
+    code: str => `"==@babel/template==";;\n${str}`,


Curious for thoughts on this. Since we want to force things to be statements and not directives, the easiest thing is just to prepend a ; to the string. I felt like it'd be a little weird for people to get errors like

SyntaxError: Unexpected token, expected ; (2:4) 1 | ; > 2 | omg 45 | ^

so I threw a little header in there so it's clearer, e.g.

SyntaxError: Unexpected token, expected ; (2:4) 1 | "==@babel/template==";; > 2 | omg 45 | ^

this certainly gives at least some context

…tilities.

loganfsmyth added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Oct 16, 2017

loganfsmyth requested a review from Jessidhia October 16, 2017 21:23

loganfsmyth commented Oct 16, 2017

View reviewed changes

hzoo reviewed Oct 17, 2017

View reviewed changes

loganfsmyth force-pushed the better-template branch from f01ae58 to 2ed4c8f Compare October 17, 2017 17:54

nicolo-ribaudo reviewed Oct 17, 2017

View reviewed changes

existentialism approved these changes Oct 17, 2017

View reviewed changes

xtuc approved these changes Oct 18, 2017

View reviewed changes

loganfsmyth commented Oct 18, 2017

View reviewed changes

loganfsmyth added 7 commits October 18, 2017 13:55

Regenerate flow types and add more types.

e6beb7c

Generate types with %checks annotations.

ad05c99

Add type declaration for t.validate.

cc22ea0

Clean up template calls.

afc3963

Default templates to sourceType:module.

fc3433c

Split template module into multiple files.

ef185fe

Add a new utility for traversing the AST.

191624d

loganfsmyth force-pushed the better-template branch from 35bae92 to 0c3804b Compare October 18, 2017 20:55

loganfsmyth added 4 commits October 18, 2017 13:58

Reimplement 'babel-template' with better caching and tagged literal u…

cc802c1

…tilities.

Use .program template for helpers.

107c377

Simplfy assertion generation.

673eaf8

Use literal-based templates for more stuff.

f230497

loganfsmyth force-pushed the better-template branch from 0c3804b to f230497 Compare October 18, 2017 20:58

loganfsmyth merged commit 97a217d into babel:master Oct 18, 2017

loganfsmyth deleted the better-template branch October 18, 2017 21:14

hzoo mentioned this pull request Oct 19, 2017

Document using babel-template as a template tag #6388

Closed

hzoo mentioned this pull request Nov 4, 2017

Add additional support for loose mode in helper-module-transforms #6742

Merged

danez mentioned this pull request Dec 16, 2017

Regression in babel-template with uppercase variables not meant to be replaced #7035

Closed

chocolateboy referenced this pull request in caesarsol/traph.macro Jan 2, 2018

🚜 Greatly improve AST generation code using babel-plugin-ast-literal

822b52c

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 bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Uh oh!

Conversation

loganfsmyth commented Oct 16, 2017

Breaking changes:

New features:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

existentialism left a comment

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xtuc left a comment

Choose a reason for hiding this comment

Uh oh!

loganfsmyth commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

babel-bot commented Oct 16, 2017 •

edited

Loading

nicolo-ribaudo Oct 17, 2017 •

edited

Loading

nicolo-ribaudo commented Oct 17, 2017 •

edited

Loading