fix: ignore tagged template strings

[toddbluhm]

What is the purpose of this pull request? (put an "X" next to item)

• Documentation update
• Bug fix
• New feature
• Other, please explain:

What changes did you make? (Give an overview)
When using the eslint config with a react project, often-times react components
are embedded in tagged template strings for ease of readability and simplicity.
React will later properly convert those tagged template strings, transforming the
components into their proper representation.

Example:

const LegendItem = styled.div`
  font-size: ${rem(25.8)};
  font-weight: 500;
  color: rgba(70, 74, 125, 0.8);
  align-items: center;
`

const Legend = styled.div`   // A `styled.div` tagged template string
  position: absolute;
  right: 3rem;
  top: 2rem;

  ${LegendItem} {            // `LegendItem` is a React Component
    margin-left: ${rem(54)};
  }
`

This PR sets the ignoreTaggedTemplateExpressions = true on the no-base-to-string rule. This causes the no-base-to-string rule to be ignored in tagged template strings.

Which issue (if any) does this pull request address?
N/A

Is there anything you'd like reviewers to focus on?
This seems like a good rule change, but perhaps tagged template strings are a bad style of programming? Or maybe there is/will be a better way to lint tagged template strings in the future?

Related to: typescript-eslint/typescript-eslint#1757
Typescript Eslint Parser Rule Documentation: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-base-to-string.md
Depends on: #273

[LinusU]

Not sure why Travis is failing 🤔 but I think this is a good change 👍

Styled Components are a one common use of this, but there are others as well...

[mightyiam]

Thank you, @toddbluhm and @LinusU.

The provided example to me seems like obviously bad practice, because there's no ability to statically analyze those strings. Or is there a tool in the React ecosystem that does that? In general, I don't like supporting obviously bad patterns. How popular is this, please?

The failure is because this option is not yet available in the version of the plugin that we use. It was introduced in 2.28.0.

https://github.com/standard/eslint-config-standard-with-typescript/blob/7226f7d084720de869bef3e64d9e78d33010b938/package.json#L74

This depends on #273.

[LinusU]

The provided example to me seems like obviously bad practice, because there's no ability to statically analyze those strings.

Tagged template literals aren't really strings though, they are literals, and unless I'm misunderstanding something they can be fully typed/statically analysed. In fact, Styled Components already does that:

styled-components/styled-components#1697

In fact, I think it's a bit weird that tagged template literals are covered by that rule at all since they should already be type checked by TypeScript. Hmm, okay, it's to "to keep this backwards-compatible" but I still think it's quite strange 🤔

ref: typescript-eslint/typescript-eslint#1757 (comment)

The failure is because this option is not yet available in the version of the plugin that we use. It was introduced in 2.28.0.

👍

[mightyiam]

The provided example to me seems like obviously bad practice, because there's no ability to statically analyze those strings.

Tagged template literals aren't really strings though, they are literals, and unless I'm misunderstanding something they can be fully typed/statically analysed. In fact, Styled Components already does that:

styled-components/styled-components#1697

I'm referring to the inability of analyzing the static parts of the string. Why does this feature even exist? Why not use objects with properties? Why strings?

[toddbluhm]

Great discussion! @LinusU I think your open issue in typescript-eslint is probably the real fix to this issue.

With that said, I am not the right person to ask about how frequently this is used or the practice behind it. I am not a frontend developer, I was just converting a project over to typescript that had some react templates in it. As a backend developer, this is the first time I have ever even heard of tagged template strings. Further research shows the feature has been apart of typescript since v1.4.

As for how popular/frequently this style of coding is, at the very least the styled-components Github project has over 28k stars and associated with over 250k other Github projects so I would say it is pretty popular and worth considering direct support for it.

My current solution right now when using ts-standard is to just put /* eslint-disable */ and /* eslint-enable */ lines around each component that embeds another component in the tagged template string. This solution is a pain, less safe, and way too verbose, hence the PR 😄

[LinusU]

I'm referring to the inability of analyzing the static parts of the string. Why does this feature even exist? Why not use objects with properties? Why strings?

Aha, well it's actually a quite common pattern in frontend JavaScript, and they actually are statically analysable with third party tools. They are basically embedding another language into JS, in this case I can agree that it might seem a bit odd if you're not coming from a CSS background. Another example of this is GraphQL which also works in the same way, and which cannot really be replaced by an object literal:

gql`
  query GetRestaurant($restaurantId: ID!, $menuType: MenuType!) {
    shoppingCart(restaurantId: $restaurantId, menuType: $menuType) @client {
      id

      entries {
        id
        count
        productId
      }
    }

    restaurant(id: $restaurantId) {
      name

      hasEatInMenu
      hasTakeAwayMenu

      menu(filter: { type: $menuType }) {
        products {
          name
          price
        }
      }
    }
  }
`

Usually there is a separate program that goes thru and checks the code, e.g. the styled components babel plugin, or the Apollo codegen tool. In the case of GraphQL there is also syntax highlighting support in VS Code, not sure about styled-components but there is no technical challenges in doing it...

---

Even so, since upstream have agreed that it shouldn't have been an option at all, and will remove it completely in the next major (3.0) I think that the correct course here is to set it to true now, and removing the option when we upgrade to 3.0 ☺️

[LinusU]

This is fixed in 16.0.0 since my PR upstream was merged, released, and we then updated to that version ☺️

ref: typescript-eslint/typescript-eslint#1916

[LinusU]

Sorry about this, I thought we updated to 2.29.0 but we did not 🙈

[mightyiam]

@LinusU master has v2.29.0. So this PR is unnecessary, right?

[LinusU]

Yes 👍

Thanks for bringing this up from the start @toddbluhm ❤️

[toddbluhm]

@LinusU @mightyiam Thanks! Glad to see this change get merged in upstream!