Add dev warning about keyframes being interpolated into plain strings. (

#1553)
emotion-js · Oct 21, 2019 · 1021195 · 1021195
1 parent 2fc75f2
commit 1021195
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 30 deletions.
diff --git a/.changeset/nine-tips-design/changes.json b/.changeset/nine-tips-design/changes.json
@@ -0,0 +1,8 @@
+{
+  "releases": [
+    { "name": "@emotion/core", "type": "patch" },
+    { "name": "@emotion/serialize", "type": "patch" },
+    { "name": "@emotion/styled", "type": "patch" }
+  ],
+  "dependents": []
+}
diff --git a/.changeset/nine-tips-design/changes.md b/.changeset/nine-tips-design/changes.md
@@ -0,0 +1 @@
+Add dev warning about keyframes being interpolated into plain strings.
diff --git a/packages/core/__tests__/warnings.js b/packages/core/__tests__/warnings.js
@@ -2,7 +2,7 @@
 /** @jsx jsx */
 import 'test-utils/next-env'
 import css from '@emotion/css'
-import { jsx, Global } from '@emotion/core'
+import { jsx, Global, keyframes } from '@emotion/core'
 import renderer from 'react-test-renderer'
 
 // $FlowFixMe
@@ -162,15 +162,15 @@ test('kebab-case', () => {
   css({ '--primary-color': 'hotpink' })
   css({ ':last-of-type': null })
   expect(console.error.mock.calls).toMatchInlineSnapshot(`
-Array [
-  Array [
-    "Using kebab-case for css properties in objects is not supported. Did you mean backgroundColor?",
-  ],
-  Array [
-    "Using kebab-case for css properties in objects is not supported. Did you mean msFilter?",
-  ],
-]
-`)
+        Array [
+          Array [
+            "Using kebab-case for css properties in objects is not supported. Did you mean backgroundColor?",
+          ],
+          Array [
+            "Using kebab-case for css properties in objects is not supported. Did you mean msFilter?",
+          ],
+        ]
+    `)
 })
 
 test('unterminated comments', () => {
@@ -206,3 +206,39 @@ test('unterminated comments', () => {
     `"Your styles have an unterminated comment (\\"/*\\" without corresponding \\"*/\\")."`
   )
 })
+
+test('keyframes interpolated into plain string', () => {
+  const animateColor = keyframes({
+    'from,to': { color: 'green' },
+    '50%': { color: 'hotpink' }
+  })
+  const rotate360 = keyframes({
+    from: {
+      transform: 'rotate(0deg)'
+    },
+    to: {
+      transform: 'rotate(360deg)'
+    }
+  })
+
+  renderer.create(
+    <div css={[`animation: ${animateColor} 10s ${rotate360} 5s;`]} />
+  )
+  expect(console.error.mock.calls).toMatchInlineSnapshot(`
+    Array [
+      Array [
+        "\`keyframes\` output got interpolated into plain string, please wrap it with \`css\`.
+
+    Instead of doing this:
+
+    const animation0 = keyframes\`{from,to{color:green;}50%{color:hotpink;}}\`
+    const animation1 = keyframes\`{from{transform:rotate(0deg);}to{transform:rotate(360deg);}}\`
+    \`animation: \${animation0} 10s \${animation1} 5s;\`
+
+    You should wrap it with \`css\` like this:
+
+    css\`animation: \${animation0} 10s \${animation1} 5s;\`",
+      ],
+    ]
+  `)
+})
diff --git a/packages/serialize/src/index.js b/packages/serialize/src/index.js
@@ -202,30 +202,57 @@ function handleInterpolation(
             "let SomeComponent = styled('div')`${dynamicStyle}`"
         )
       }
+      break
     }
-    // eslint-disable-next-line no-fallthrough
-    default: {
-      if (registered == null) {
-        return interpolation
-      }
-      const cached = registered[interpolation]
-      if (
-        process.env.NODE_ENV !== 'production' &&
-        couldBeSelectorInterpolation &&
-        shouldWarnAboutInterpolatingClassNameFromCss &&
-        cached !== undefined
-      ) {
-        console.error(
-          'Interpolating a className from css`` is not recommended and will cause problems with composition.\n' +
-            'Interpolating a className from css`` will be completely unsupported in a future major version of Emotion'
+    case 'string':
+      if (process.env.NODE_ENV !== 'production') {
+        const matched = []
+        const replaced = interpolation.replace(
+          animationRegex,
+          (match, p1, p2) => {
+            const fakeVarName = `animation${matched.length}`
+            matched.push(
+              `const ${fakeVarName} = keyframes\`${p2.replace(
+                /^@keyframes animation-\w+/,
+                ''
+              )}\``
+            )
+            return `\${${fakeVarName}}`
+          }
         )
-        shouldWarnAboutInterpolatingClassNameFromCss = false
+        if (matched.length) {
+          console.error(
+            '`keyframes` output got interpolated into plain string, please wrap it with `css`.\n\n' +
+              'Instead of doing this:\n\n' +
+              [...matched, `\`${replaced}\``].join('\n') +
+              '\n\nYou should wrap it with `css` like this:\n\n' +
+              `css\`${replaced}\``
+          )
+        }
       }
-      return cached !== undefined && !couldBeSelectorInterpolation
-        ? cached
-        : interpolation
-    }
+      break
+  }
+
+  // finalize string values (regular strings and functions interpolated into css calls)
+  if (registered == null) {
+    return interpolation
+  }
+  const cached = registered[interpolation]
+  if (
+    process.env.NODE_ENV !== 'production' &&
+    couldBeSelectorInterpolation &&
+    shouldWarnAboutInterpolatingClassNameFromCss &&
+    cached !== undefined
+  ) {
+    console.error(
+      'Interpolating a className from css`` is not recommended and will cause problems with composition.\n' +
+        'Interpolating a className from css`` will be completely unsupported in a future major version of Emotion'
+    )
+    shouldWarnAboutInterpolatingClassNameFromCss = false
   }
+  return cached !== undefined && !couldBeSelectorInterpolation
+    ? cached
+    : interpolation
 }
 
 function createStringFromObject(