no-unused-expressions and template literals (`)

[kachkaev]

Looks like template literals with custom interpolators with no brackets are not treated by eslint as proper function calls. The following example produces an error:

import injectGlobal from 'styled-components';

// OK (with brackets)
injectGlobal(`
  body {
    color: #000,
  }
`);

// causes eslint error (no brackets)
injectGlobal`
  body {
    background: #f00,
  }
`;

11:1  error  Expected an assignment or function call and instead saw an expression  no-unused-expressions

• ESLint Version: 3.10.2
• Node Version: 6.9.1
• npm Version: 3.10.8

What parser (default, Babel-ESLint, etc.) are you using? babel-eslint 7.1.1

Please show your full configuration:

  "eslintConfig": {
    "parser": "babel-eslint",
    "plugins": [
      "import"
    ],
    "rules": {
      "no-unused-expressions": 2
    }
  }

What did you do? Please include the actual source code causing the issue.

MWE: https://github.com/kachkaev/eslint-no-unused-expressions

git clone git@github.com:kachkaev/eslint-no-unused-expressions.git
npm install
npm run lint:eslint

What did you expect to happen?

I expected both injectGlobal( and injectGlobal not to produce any eslint errors as these are just normal function calls; they are identical to each other.

[kachkaev]

UPD: Looks like I was wrong assuming injectGlobal( and injectGlobal are 100% identical – the string gets concatenated without a custom interpolator in the first case. The example above should be changed to this:

// OK (with brackets)
injectGlobal([`
  body {
    color: #000,
  }
`]);

// causes eslint error (no brackets)
injectGlobal`
  body {
    background: #f00,
  }
`;

This does not cancel the issue though.

[platinumazure]

I'm treating this as a rule enhancement request, since we probably never thought about whether template tags should or shouldn't be regarded as an expression with side effects.

I think it makes sense to enhance the rule in this way. 👍 from me.

[not-an-aardvark]

I'm not sure about this.

I agree that the tag function does get called and might have side effects, so reporting it as an unused expression could be considered a bug.

On the other hand, while I haven't used tagged templates extensively, my understanding is that it's good practice for template tags to be pure functions, which would imply that they avoid side effects. So maybe the rule should report template tag functions, the same way that it reports getters:

// This might have side effects if foo.bar is a getter.
// But a getter with side effects is usually a code smell,
// and this is likely to be an error.
foo.bar;

I think the decision should come down to whether there is common use-case for tagged templates with side effects.

[kachkaev]

I can name at least two somewhat popular libraries where template tags have side effects.
The first one is styled-components, from which comes that injectGlobal tag mentioned above. The second one is graphql-tag, which is a part of apollo-client. Although gql tag is normally used in Apollo as an expression that returns something, it does create a side effect related to reading and writing cache.

After thinking about linting template tags for a bit, I realised that the best scenario for me as a user would be to treat templateTags as a special case, similarly to how this is currently done with allowShortCircut and allowTernary. But in addition to just true and false, one should probably be able to white-list only certain tags:

["error", { "allowTernary": true, "allowTemplateTags": true}]
["error", { "allowTernary": true, "allowTemplateTags": ["injectGlobal"]}]

The second rule will work like this:

injectGlobal`body{ color: red; }`; // OK (semantically meaningful)
gql`...some graphql query...`; // error (warms up Apollo cache, but unlikely to be useful)

[platinumazure]

I could get behind an option as well, so that the user is at least fully in control over how the rule will enforce template tags.

[not-an-aardvark]

👍 for adding allowTemplateTags: true.

👎 for adding allowTemplateTags: ["foo", "bar"]

My gut impression is that adding an option like allowTemplateTags: ["injectGlobal"] might be excessive. It would have to use the static name for the tag, so it wouldn't work if the function was renamed within the code. It would also fail for cases like this:

/* eslint no-unused-expressions: [error, {"allowTemplateTags": ["injectGlobal", "injectLocal"]}] */

(foo ? injectGlobal : injectLocal)`
  body {
    color: red;
  }
` // incorrectly reports an error

var foo = {inject: injectGlobal};

foo.inject`
  body {
    color: red;
  }
` // incorrectly reports an error

We don't have an option like allowTernary: ["foo", "bar"], presumably for similar reasons. I think the small utility provided by allowing an array isn't worth having the rule incorrectly report errors in more complicated scenarios.

[kachkaev]

@not-an-aardvark I understand the limitations of the second case and also think that you've demonstrated them quite well. Nevertheless IMO the existence of an option to create a white list does not harm. Those who want to have dynamic template tags may just stick with allowTemplateTags: true.

Those two libraries I mentioned suggest a very simple usage scenario: you just import a specific template literal and then apply it once or a couple of times. There is no space for dynamic naming really (from the semantic point of view, not a technical one).

[dead-claudia]

I could use this as well. I've created a simple template tag for debug logging (with object pretty-printing) specific for a utility I'm writing, and having to invoke it each time would quickly get cumbersome. I'd rather not have to convert this into a variadic function.

const {zip} = require("lodash")
const {inspect} = require("util")

function debug(parts, ...args) {
    const items = [parts[0]].concat(...zip(args.map(arg => inspect(arg)), parts.slice(1)))
    console.error("module-name %d: %s", process.pid, items.join(""))
}

// example usage
debug `invoke module: ${module.name}, method: ${method.name}`

// variadic, way uglier
function debug(...args) {
    console.error(`module-name ${process.pid} ${
        args.map(arg => typeof arg === "string" ? arg : inspect(arg)).join("")
    }`)
}

debug("invoke module: ", module.name, ", method: ", method.name)

[mysticatea]

Hmm, I felt it's abuse of tagged templates since I think "template" is not designed to intend as having side effects.
Though I will not actively oppose allowTaggedTemplates-like option (off by default)...

[dead-claudia]

@mysticatea I feel the main purpose of template tags are for DSLs. Some DSLs are inherently stateful (e.g. running SQL queries, shell commands), while others are not (e.g. DOM generation, internationalization), and I feel both should be okay. We may agree to disagree here on whether template tags should be allowed to have side effects, but the stateful use cases like above are why I'm interested in adding an option like this.

[martijndeh]

I see the evaluating label is still set for this one and not many people 👍'ed the allowTemplateTags: true yet so I want to give another example.

I use a library of mine to construct queries and it's using template literals extensively. In one case, it's creating a side effect which now gets flagged as no-unused-expressions by eslint. Personally, I see the tagged template as a special function call with the template as argument (which may or may not return a value) instead of a getter.

export function up(transaction) {
    // transaction is a queue which executes queries in series
    transaction.sql `CREATE TABLE accounts (
	...
    )`;
}

I'm willing to work on this if needed.

[nzakas]

Team we need a champion for this or else it's time to close the issue.

[platinumazure]

I'll champion this, for allowTemplateTags: true. (I agree with @not-an-aardvark's comment about local renaming.)

I've removed my 👍 on the rule comment since I'm now championing the rule. We need two more 👍s to get this accepted.

[jhitchins88]

I'm down for this rule. Do you need assistance getting it to work?

[not-an-aardvark]

It looks like we need one more 👍 from an ESLint team member in order to mark the change as "accepted". @jhitchins88, we'd certainly appreciate a PR, although it might be better to wait until this issue is marked as accepted so that you don't waste your time in the event that this issue doesn't get accepted.

[platinumazure]

@eslint/eslint-team Anyone else want to 👍 this issue so we can accept it? (At this point, goal is to implement an allowTemplateTags: true option for no-useless-expression to unconditionally count all template tags as non-useless. We can revisit a whitelist concept later, if need be.)

@kachkaev @isiahmeadows @jhitchins88 Would any of you be interested in implementing, if this is accepted?

[vitorbal]

I'll support this 👍

[platinumazure]

Working on this now.