Fix invalid rules break stylesheet/registry (#296)

* Fix invalid rules break stylesheet/registry * comments
vercel · Oct 7, 2017 · bf1a0a2 · bf1a0a2
1 parent c87bac3
commit bf1a0a2
Show file tree

Hide file tree

Showing 5 changed files with 128 additions and 135 deletions.
diff --git a/src/lib/stylesheet.js b/src/lib/stylesheet.js
@@ -126,6 +126,7 @@ export default class StyleSheet {
             `StyleSheet: illegal rule: \n\n${rule}\n\nSee https://stackoverflow.com/q/20007992 for more info`
           ) // eslint-disable-line no-console
         }
+        return -1
       }
     } else {
       const insertionPoint = this._tags[index]
@@ -141,8 +142,25 @@ export default class StyleSheet {
       if (!rule.trim()) {
         rule = this._deletedRulePlaceholder
       }
+
+      if (!sheet.cssRules[index]) {
+        // @TBD Should we throw an error?
+        return index
+      }
+
       sheet.deleteRule(index)
-      sheet.insertRule(rule, index)
+
+      try {
+        sheet.insertRule(rule, index)
+      } catch (err) {
+        if (!isProd) {
+          console.warn(
+            `StyleSheet: illegal rule: \n\n${rule}\n\nSee https://stackoverflow.com/q/20007992 for more info`
+          ) // eslint-disable-line no-console
+        }
+        // In order to preserve the indices we insert a deleteRulePlaceholder
+        sheet.insertRule(this._deletedRulePlaceholder, index)
+      }
     } else {
       const tag = this._tags[index]
       invariant(tag, `old rule at index \`${index}\` not found`)

diff --git a/src/stylesheet-registry.js b/src/stylesheet-registry.js
@@ -51,8 +51,15 @@ export default class StyleSheetRegistry {
       return
     }
 
-    this._instancesCounts[styleId] = 1
-    this._indices[styleId] = rules.map(rule => this._sheet.insertRule(rule))
+    const indices = rules
+      .map(rule => this._sheet.insertRule(rule))
+      // Filter out invalid rules
+      .filter(index => index !== -1)
+
+    if (indices.length > 0) {
+      this._indices[styleId] = indices
+      this._instancesCounts[styleId] = 1
+    }
   }
 
   remove(props) {

diff --git a/test/stylesheet-registry.js b/test/stylesheet-registry.js
@@ -3,7 +3,7 @@ import test from 'ava'
 
 // Ours
 import StyleSheetRegistry from '../src/stylesheet-registry'
-import makeSheet from './stylesheet'
+import makeSheet, { invalidRules } from './stylesheet'
 
 function makeRegistry(options) {
   const registry = new StyleSheetRegistry({
@@ -20,24 +20,7 @@ const cssRuleAlt = 'p { color: red }'
 // registry.add
 
 test('add', t => {
-  const options = [
-    {
-      optimizeForSpeed: true,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: true,
-      isBrowser: false
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: false
-    }
-  ]
+  const options = [{ optimizeForSpeed: true, isBrowser: true }, { optimizeForSpeed: false, isBrowser: true }, { optimizeForSpeed: true, isBrowser: false }, { optimizeForSpeed: false, isBrowser: false }]
 
   options.forEach(options => {
     const registry = makeRegistry(options)
@@ -68,10 +51,7 @@ test('add', t => {
     ])
 
     if (options.optimizeForSpeed) {
-      registry.add({
-        styleId: '456',
-        css: [cssRule, cssRuleAlt]
-      })
+      registry.add({ styleId: '456', css: [cssRule, cssRuleAlt] })
 
       t.deepEqual(registry.cssRules(), [
         ['jsx-123', cssRule],
@@ -82,27 +62,28 @@ test('add', t => {
   })
 })
 
+test('add - filters out invalid rules (index `-1`)', t => {
+  const registry = makeRegistry()
+
+  // Insert a valid rule
+  registry.add({ styleId: '123', css: [cssRule] })
+
+  // Insert an invalid rule
+  registry.add({ styleId: '456', css: [invalidRules[0]] })
+
+  // Insert another valid rule
+  registry.add({ styleId: '678', css: [cssRule] })
+
+  t.deepEqual(registry.cssRules(), [
+    ['jsx-123', 'div { color: red }'],
+    ['jsx-678', 'div { color: red }']
+  ])
+})
+
 // registry.remove
 
 test('remove', t => {
-  const options = [
-    {
-      optimizeForSpeed: true,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: true,
-      isBrowser: false
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: false
-    }
-  ]
+  const options = [{ optimizeForSpeed: true, isBrowser: true }, { optimizeForSpeed: false, isBrowser: true }, { optimizeForSpeed: true, isBrowser: false }, { optimizeForSpeed: false, isBrowser: false }]
 
   options.forEach(options => {
     const registry = makeRegistry(options)
@@ -138,24 +119,7 @@ test('remove', t => {
 // registry.update
 
 test('update', t => {
-  const options = [
-    {
-      optimizeForSpeed: true,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: true,
-      isBrowser: false
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: false
-    }
-  ]
+  const options = [{ optimizeForSpeed: true, isBrowser: true }, { optimizeForSpeed: false, isBrowser: true }, { optimizeForSpeed: true, isBrowser: false }, { optimizeForSpeed: false, isBrowser: false }]
 
   options.forEach(options => {
     const registry = makeRegistry(options)
@@ -169,13 +133,7 @@ test('update', t => {
       css: options.optimizeForSpeed ? [cssRule] : cssRule
     })
 
-    registry.update(
-      { styleId: '123' },
-      {
-        styleId: '345',
-        css: options.optimizeForSpeed ? [cssRuleAlt] : cssRuleAlt
-      }
-    )
+    registry.update({ styleId: '123' }, { styleId: '345', css: options.optimizeForSpeed ? [cssRuleAlt] : cssRuleAlt })
     // Doesn't remove when there are multiple instances of 123
     t.deepEqual(registry.cssRules(), [
       ['jsx-123', cssRule],
@@ -186,13 +144,7 @@ test('update', t => {
     t.deepEqual(registry.cssRules(), [['jsx-123', cssRule]])
 
     // Update again
-    registry.update(
-      { styleId: '123' },
-      {
-        styleId: '345',
-        css: options.optimizeForSpeed ? [cssRuleAlt] : cssRuleAlt
-      }
-    )
+    registry.update({ styleId: '123' }, { styleId: '345', css: options.optimizeForSpeed ? [cssRuleAlt] : cssRuleAlt })
     // 123 replaced with 345
     t.deepEqual(registry.cssRules(), [['jsx-345', cssRuleAlt]])
   })
@@ -219,13 +171,7 @@ test('createComputeId', t => {
 test('createComputeSelector', t => {
   const computeSelector = utilRegistry.createComputeSelector()
 
-  t.is(
-    computeSelector(
-      'jsx-123',
-      '.test {} .__jsx-style-dynamic-selector { color: red } .__jsx-style-dynamic-selector { color: red }'
-    ),
-    '.test {} .jsx-123 { color: red } .jsx-123 { color: red }'
-  )
+  t.is(computeSelector('jsx-123', '.test {} .__jsx-style-dynamic-selector { color: red } .__jsx-style-dynamic-selector { color: red }'), '.test {} .jsx-123 { color: red } .jsx-123 { color: red }')
 })
 
 // getIdAndRules

diff --git a/test/stylesheet.js b/test/stylesheet.js
@@ -4,6 +4,8 @@ import test from 'ava'
 // Ours
 import StyleSheet from '../src/lib/stylesheet'
 
+export const invalidRules = ['invalid rule']
+
 export default function makeSheet(
   options = { optimizeForSpeed: true, isBrowser: true }
 ) {
@@ -15,6 +17,9 @@ export default function makeSheet(
       sheet: {
         cssRules,
         insertRule: (rule, index) => {
+          if (invalidRules.includes(rule)) {
+            throw new Error('invalid rule')
+          }
           if (typeof index === 'number') {
             cssRules[index] = { cssText: rule }
           } else {
@@ -79,20 +84,7 @@ test('can change optimizeForSpeed only when the stylesheet is empty', t => {
 // sheet.insertRule
 
 test('insertRule', t => {
-  const options = [
-    {
-      optimizeForSpeed: true,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: true,
-      isBrowser: false
-    }
-  ]
+  const options = [{ optimizeForSpeed: true, isBrowser: true }, { optimizeForSpeed: false, isBrowser: true }, { optimizeForSpeed: true, isBrowser: false }]
 
   options.forEach(options => {
     const sheet = makeSheet(options)
@@ -120,27 +112,18 @@ test('insertRule - returns the rule index', t => {
   t.is(i, 1)
 })
 
+test('insertRule - handles invalid rules and returns -1 as index', t => {
+  const sheet = makeSheet()
+  sheet.inject()
+
+  const i = sheet.insertRule(invalidRules[0])
+  t.is(i, -1)
+})
+
 // sheet.deleteRule
 
 test('deleteRule', t => {
-  const options = [
-    {
-      optimizeForSpeed: true,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: true,
-      isBrowser: false
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: false
-    }
-  ]
+  const options = [{ optimizeForSpeed: true, isBrowser: true }, { optimizeForSpeed: false, isBrowser: true }, { optimizeForSpeed: true, isBrowser: false }, { optimizeForSpeed: false, isBrowser: false }]
 
   options.forEach(options => {
     const sheet = makeSheet(options)
@@ -158,27 +141,19 @@ test('deleteRule', t => {
   })
 })
 
+test('deleteRule - does not throw when the rule at index does not exist', t => {
+  const sheet = makeSheet()
+  sheet.inject()
+
+  t.notThrows(() => {
+    sheet.deleteRule(sheet.length + 1)
+  })
+})
+
 // sheet.replaceRule
 
 test('replaceRule', t => {
-  const options = [
-    {
-      optimizeForSpeed: true,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: true
-    },
-    {
-      optimizeForSpeed: true,
-      isBrowser: false
-    },
-    {
-      optimizeForSpeed: false,
-      isBrowser: false
-    }
-  ]
+  const options = [{ optimizeForSpeed: true, isBrowser: true }, { optimizeForSpeed: false, isBrowser: true }, { optimizeForSpeed: true, isBrowser: false }, { optimizeForSpeed: false, isBrowser: false }]
 
   options.forEach(options => {
     const sheet = makeSheet(options)
@@ -190,3 +165,22 @@ test('replaceRule', t => {
     t.deepEqual(sheet.cssRules(), [{ cssText: 'p { color: hotpink }' }])
   })
 })
+
+test('replaceRule - handles invalid rules gracefully', t => {
+  const sheet = makeSheet()
+  sheet.inject()
+
+  // Insert two rules
+  sheet.insertRule('div { color: red }')
+  const index = sheet.insertRule('div { color: red }')
+
+  // Replace the latter with an invalid rule
+  const i = sheet.replaceRule(index, invalidRules[0])
+  t.is(i, index)
+  t.is(sheet.length, 2)
+
+  // Even though replacement (insertion) failed deletion succeeded
+  // therefore the lib must insert a delete placeholder which resolves to `null`
+  // when `cssRules()` is called.
+  t.deepEqual(sheet.cssRules(), [{ cssText: 'div { color: red }' }, null])
+})