Delete rule perf (#1424)

* Update changelog.md * impove deleteRule performance * add changelog
cssinjs · Nov 13, 2020 · 47daec4 · 47daec4
1 parent cf1b714
commit 47daec4
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 32 deletions.
diff --git a/changelog.md b/changelog.md
@@ -7,6 +7,14 @@ Since you are interested in what happens next, in case, you work for a for-profi
 - [jss] Restores TypeScript support for Observable styles [1402](https://github.com/cssinjs/jss/pull/1402)
 - [jss-plugin-default-unit] Fix missing default unit for 0ms and 0% [1413](https://github.com/cssinjs/jss/pull/1413)
 
+### Improvements
+
+- [*] Improved docs [1384](https://github.com/cssinjs/jss/pull/1384), [1387](https://github.com/cssinjs/jss/pull/1387), [1391](https://github.com/cssinjs/jss/pull/1391),
+- [*] Remove test files from the package [1406](https://github.com/cssinjs/jss/pull/1406)
+- [jss-plugin-default-unit] aAdd gap unit [1403](https://github.com/cssinjs/jss/pull/1403)
+- [jss-plugin-default-unit] Add default units to logical properties [1415](https://github.com/cssinjs/jss/pull/1415)
+- [jss] Improve deleteRule() performance [1424](https://github.com/cssinjs/jss/pull/1424)
+
 ---
 
 ## 10.4.0 (2020-8-14)

diff --git a/packages/jss/.size-snapshot.json b/packages/jss/.size-snapshot.json
@@ -1,30 +1,30 @@
 {
   "dist/jss.js": {
-    "bundled": 61966,
-    "minified": 22868,
-    "gzipped": 6898
+    "bundled": 62568,
+    "minified": 22988,
+    "gzipped": 6925
   },
   "dist/jss.min.js": {
-    "bundled": 60569,
-    "minified": 22100,
-    "gzipped": 6545
+    "bundled": 61171,
+    "minified": 22220,
+    "gzipped": 6572
   },
   "dist/jss.cjs.js": {
-    "bundled": 56677,
-    "minified": 24785,
-    "gzipped": 6896
+    "bundled": 57259,
+    "minified": 24977,
+    "gzipped": 6934
   },
   "dist/jss.esm.js": {
-    "bundled": 56145,
-    "minified": 24350,
-    "gzipped": 6805,
+    "bundled": 56727,
+    "minified": 24542,
+    "gzipped": 6843,
     "treeshaked": {
       "rollup": {
-        "code": 20081,
+        "code": 20201,
         "import_statements": 345
       },
       "webpack": {
-        "code": 21562
+        "code": 21682
       }
     }
   }

diff --git a/packages/jss/src/DomRenderer.js b/packages/jss/src/DomRenderer.js
@@ -260,15 +260,8 @@ const getNonce = memoize(
 const insertRule = (
   container: CSSStyleSheet | CSSMediaRule | CSSKeyframesRule,
   rule: string,
-  index?: number
+  index: number
 ): false | any => {
-  const maxIndex = container.cssRules.length
-  // In case previous insertion fails, passed index might be wrong
-  if (index === undefined || index > maxIndex) {
-    // eslint-disable-next-line no-param-reassign
-    index = maxIndex
-  }
-
   try {
     if ('insertRule' in container) {
       const c = ((container: any): CSSStyleSheet)
@@ -286,6 +279,19 @@ const insertRule = (
   return container.cssRules[index]
 }
 
+const getValidRuleInsertionIndex = (
+  container: CSSStyleSheet | CSSMediaRule | CSSKeyframesRule,
+  index?: number
+): number => {
+  const maxIndex = container.cssRules.length
+  // In case previous insertion fails, passed index might be wrong
+  if (index === undefined || index > maxIndex) {
+    // eslint-disable-next-line no-param-reassign
+    return maxIndex
+  }
+  return index
+}
+
 const createStyle = (): HTMLElement => {
   const el = document.createElement('style')
   // Without it, IE will have a broken source order specificity if we
@@ -311,6 +317,10 @@ export default class DomRenderer {
 
   hasInsertedRules: boolean = false
 
+  // Will be empty if link: true option is not set, because
+  // it is only for use together with insertRule API.
+  cssRules: AnyCSSRule[] = []
+
   constructor(sheet?: StyleSheet) {
     // There is no sheet when the renderer is used from a standalone StyleRule.
     if (sheet) sheets.add(sheet)
@@ -386,8 +396,13 @@ export default class DomRenderer {
       const parent: ContainerRule = (rule: any)
       let latestNativeParent = nativeParent
       if (rule.type === 'conditional' || rule.type === 'keyframes') {
+        const insertionIndex = getValidRuleInsertionIndex(nativeParent, index)
         // We need to render the container without children first.
-        latestNativeParent = insertRule(nativeParent, parent.toString({children: false}), index)
+        latestNativeParent = insertRule(
+          nativeParent,
+          parent.toString({children: false}),
+          insertionIndex
+        )
         if (latestNativeParent === false) {
           return false
         }
@@ -407,13 +422,15 @@ export default class DomRenderer {
 
     if (!ruleStr) return false
 
-    const nativeRule = insertRule(nativeParent, ruleStr, index)
+    const insertionIndex = getValidRuleInsertionIndex(nativeParent, index)
+    const nativeRule = insertRule(nativeParent, ruleStr, insertionIndex)
     if (nativeRule === false) {
       return false
     }
 
     this.hasInsertedRules = true
     rule.renderable = nativeRule
+    this.cssRules[insertionIndex] = nativeRule
     return nativeRule
   }
 
@@ -425,18 +442,15 @@ export default class DomRenderer {
     const index = this.indexOf(cssRule)
     if (index === -1) return false
     sheet.deleteRule(index)
+    this.cssRules.splice(index, 1)
     return true
   }
 
   /**
    * Get index of a CSS Rule.
    */
   indexOf(cssRule: AnyCSSRule): number {
-    const {cssRules} = this.element.sheet
-    for (let index = 0; index < cssRules.length; index++) {
-      if (cssRule === cssRules[index]) return index
-    }
-    return -1
+    return this.cssRules.indexOf(cssRule)
   }
 
   /**
@@ -448,6 +462,7 @@ export default class DomRenderer {
     const index = this.indexOf(cssRule)
     if (index === -1) return false
     this.element.sheet.deleteRule(index)
+    this.cssRules.splice(index, 1)
     return this.insertRule(rule, index)
   }
 

diff --git a/packages/jss/src/StyleSheet.js b/packages/jss/src/StyleSheet.js
@@ -154,7 +154,14 @@ export default class StyleSheet {
   deleteRule(name: string | Rule): boolean {
     const rule = typeof name === 'object' ? name : this.rules.get(name)
 
-    if (!rule) return false
+    if (
+      !rule ||
+      // Style sheet was created without link: true and attached, in this case we
+      // won't be able to remove the CSS rule from the DOM.
+      (this.attached && !rule.renderable)
+    ) {
+      return false
+    }
 
     this.rules.remove(rule)
 

diff --git a/packages/jss/tests/functional/sheet.js b/packages/jss/tests/functional/sheet.js
@@ -405,10 +405,11 @@ describe('Functional: sheet', () => {
   describe('.deleteRule()', () => {
     it('should delete a rule from the sheet and DOM', () => {
       const sheet = jss.createStyleSheet({a: {width: '1px'}}, {link: true}).attach()
-      expect(computeStyle(sheet.classes.a).width).to.be('1px')
+      const className = sheet.classes.a
+      expect(computeStyle(className).width).to.be('1px')
       expect(sheet.deleteRule('a')).to.be(true)
       expect(sheet.getRule('a')).to.be(undefined)
-      expect(computeStyle(sheet.classes.a).width).not.to.be('1px')
+      expect(computeStyle(className).width).not.to.be('1px')
       sheet.detach()
     })
   })