fix memory leak in dynamic nested rule (#1563)

* fix memory leak by function nested rule

* add test against function nested rule memory leak

* add test for update

to test both addRule and replaceRule

* add functional tests for replaceRule

* add integration tests for replaceRule

* add test for identical nest (just '&')

* add tests for multiple updates with function-nested

* refactor code, reduce bundle size

* make test description more meaningful

Co-authored-by: Oleg Isonen <oleg008@gmail.com>

* make test description more meaningful

Co-authored-by: Oleg Isonen <oleg008@gmail.com>

* make test descriptions clear

* update test decription

* prevent unnecessary copy

* remove RuleList.getByName, StyleSheet.getByName

include related change
- use almost unique name rather than selector name in nested plugin to prevent accidental replacement
- fix strange behavior of global plugin (probably bug)

* make replaceRule to return just a newRule

* add tests for global container rule

Co-authored-by: Oleg Isonen <oleg008@gmail.com>

Author: seiyab

packages/css-jss/.size-snapshot.json

@@ -1,13 +1,13 @@
 {
   "css-jss.js": {
-    "bundled": 59688,
-    "minified": 21828,
-    "gzipped": 7357
+    "bundled": 60043,
+    "minified": 21979,
+    "gzipped": 7382
   },
   "css-jss.min.js": {
-    "bundled": 58613,
-    "minified": 21205,
-    "gzipped": 7078
+    "bundled": 58968,
+    "minified": 21356,
+    "gzipped": 7102
   },
   "css-jss.cjs.js": {
     "bundled": 2949,

packages/jss-plugin-global/.size-snapshot.json

@@ -1,23 +1,23 @@
 {
   "jss-plugin-global.js": {
-    "bundled": 4988,
-    "minified": 2184,
-    "gzipped": 919
+    "bundled": 5294,
+    "minified": 2313,
+    "gzipped": 940
   },
   "jss-plugin-global.min.js": {
-    "bundled": 4988,
-    "minified": 2184,
-    "gzipped": 919
+    "bundled": 5294,
+    "minified": 2313,
+    "gzipped": 940
   },
   "jss-plugin-global.cjs.js": {
-    "bundled": 4263,
-    "minified": 2310,
-    "gzipped": 892
+    "bundled": 4551,
+    "minified": 2439,
+    "gzipped": 911
   },
   "jss-plugin-global.esm.js": {
-    "bundled": 3917,
-    "minified": 2036,
-    "gzipped": 788,
+    "bundled": 4205,
+    "minified": 2165,
+    "gzipped": 806,
     "treeshaked": {
       "rollup": {
         "code": 55,

packages/jss-plugin-global/src/index.js

@@ -41,6 +41,15 @@ class GlobalContainerRule {
     return rule
   }
 
+  /**
+   * Replace rule, run plugins.
+   */
+  replaceRule(name, style, options) {
+    const newRule = this.rules.replace(name, style, options)
+    if (newRule) this.options.jss.plugins.onProcessRule(newRule)
+    return newRule
+  }
+
   /**
    * Get index of a rule.
    */
@@ -146,7 +155,7 @@ export default function jssGlobal() {
       }
     }
 
-    if (options.scoped === false) {
+    if (!options.selector && options.scoped === false) {
       options.selector = name
     }
 

packages/jss-plugin-global/src/index.test.js

@@ -6,7 +6,7 @@ import nested from 'jss-plugin-nested'
 import global from './index'
 
 const settings = {
-  createGenerateId: () => (rule) => `${rule.key}-id`
+  createGenerateId: () => rule => `${rule.key}-id`
 }
 
 describe('jss-plugin-global', () => {
@@ -37,6 +37,94 @@ describe('jss-plugin-global', () => {
     it('should generate correct CSS', () => {
       expect(sheet.toString()).to.be('a {\n  color: red;\n}\nbody {\n  color: green;\n}')
     })
+
+    describe('addRule', () => {
+      let globalRule
+      beforeEach(() => {
+        globalRule = sheet.getRule('@global')
+        globalRule.addRule('button', {margin: 0})
+        globalRule.addRule('li', {float: 'left'})
+      })
+
+      it('should add rule', () => {
+        expect(globalRule.getRule('button')).to.not.be(undefined)
+        expect(globalRule.getRule('li')).to.not.be(undefined)
+      })
+
+      it('should generate correct CSS', () => {
+        expect(sheet.toString()).to.be(stripIndent`
+          a {
+            color: red;
+          }
+          body {
+            color: green;
+          }
+          button {
+            margin: 0;
+          }
+          li {
+            float: left;
+          }
+        `)
+      })
+    })
+
+    describe('replaceRule', () => {
+      let globalRule
+      let previousA
+      beforeEach(() => {
+        globalRule = sheet.getRule('@global')
+        previousA = globalRule.getRule('a')
+        globalRule.replaceRule('a', {color: 'yellow'})
+        globalRule.replaceRule('li', {float: 'left'})
+      })
+
+      it('should replace and add rule', () => {
+        expect(globalRule.getRule('a')).to.not.be(previousA)
+        expect(globalRule.getRule('li')).to.not.be(undefined)
+      })
+
+      it('should generate correct CSS', () => {
+        expect(sheet.toString()).to.be(stripIndent`
+          a {
+            color: yellow;
+          }
+          body {
+            color: green;
+          }
+          li {
+            float: left;
+          }
+        `)
+      })
+    })
+
+    describe('addRule / replaceRule with selector', () => {
+      let globalRule
+      beforeEach(() => {
+        globalRule = sheet.getRule('@global')
+        globalRule.addRule('arbitrary-name-1', {color: 'red'}, {selector: 'span'})
+        globalRule.replaceRule('arbitrary-name-2', {float: 'left'}, {selector: 'ul'})
+        globalRule.replaceRule('a', {display: 'block'}, {selector: 'div'})
+      })
+
+      it('should generate correct CSS', () => {
+        expect(sheet.toString()).to.be(stripIndent`
+          div {
+            display: block;
+          }
+          body {
+            color: green;
+          }
+          span {
+            color: red;
+          }
+          ul {
+            float: left;
+          }
+        `)
+      })
+    })
   })
 
   describe('@global linked', () => {

packages/jss-plugin-nested/.size-snapshot.json

@@ -1,23 +1,23 @@
 {
   "jss-plugin-nested.js": {
-    "bundled": 4456,
-    "minified": 1618,
-    "gzipped": 881
+    "bundled": 4505,
+    "minified": 1640,
+    "gzipped": 894
   },
   "jss-plugin-nested.min.js": {
-    "bundled": 4016,
-    "minified": 1402,
-    "gzipped": 754
+    "bundled": 4065,
+    "minified": 1424,
+    "gzipped": 768
   },
   "jss-plugin-nested.cjs.js": {
-    "bundled": 3729,
-    "minified": 1578,
-    "gzipped": 808
+    "bundled": 3776,
+    "minified": 1600,
+    "gzipped": 822
   },
   "jss-plugin-nested.esm.js": {
-    "bundled": 3310,
-    "minified": 1255,
-    "gzipped": 693,
+    "bundled": 3357,
+    "minified": 1277,
+    "gzipped": 705,
     "treeshaked": {
       "rollup": {
         "code": 64,

packages/jss-plugin-nested/src/index.js

@@ -18,9 +18,8 @@ export default function jssNested() {
 
       warning(
         false,
-        `[JSS] Could not find the referenced rule "${key}" in "${
-          container.options.meta || container.toString()
-        }".`
+        `[JSS] Could not find the referenced rule "${key}" in "${container.options.meta ||
+          container.toString()}".`
       )
       return key
     }
@@ -88,7 +87,8 @@ export default function jssNested() {
         // Replace all $refs.
         selector = selector.replace(refRegExp, replaceRef)
 
-        container.addRule(selector, style[prop], {...options, selector})
+        const name = `${styleRule.key}-${prop}`
+        container.replaceRule(name, style[prop], {...options, selector})
       } else if (isNestedConditional) {
         // Place conditional right after the parent rule to ensure right ordering.
         container

packages/jss-plugin-nested/src/index.test.js

@@ -9,7 +9,7 @@ import functionPlugin from 'jss-plugin-rule-value-function'
 import nested from '.'
 
 const settings = {
-  createGenerateId: () => (rule) => `${rule.key}-id`
+  createGenerateId: () => rule => `${rule.key}-id`
 }
 
 describe('jss-plugin-nested', () => {
@@ -127,6 +127,85 @@ describe('jss-plugin-nested', () => {
     })
   })
 
+  describe('identical nest', () => {
+    describe('single nest', () => {
+      let sheet
+
+      beforeEach(() => {
+        sheet = jss.createStyleSheet({
+          a: {
+            float: 'left',
+            '&': {color: 'blue'}
+          }
+        })
+      })
+
+      it('should add rule', () => {
+        expect(sheet.getRule('a')).to.not.be(undefined)
+      })
+
+      it('should generate correct CSS', () => {
+        expect(sheet.toString()).to.be(stripIndent`
+        .a-id {
+          float: left;
+        }
+        .a-id {
+          color: blue;
+        }
+      `)
+      })
+    })
+
+    describe('deep nest', () => {
+      let sheet
+
+      beforeEach(() => {
+        sheet = jss.createStyleSheet({
+          a: {
+            float: 'left',
+            '&': {
+              color: 'blue',
+              '&': {
+                width: 0,
+                '&.b': {
+                  'z-index': 1,
+                  '&': {
+                    top: 0
+                  }
+                }
+              }
+            }
+          }
+        })
+      })
+
+      it('should add rules', () => {
+        expect(sheet.getRule('a')).to.not.be(undefined)
+        expect(sheet.getRule('.a-id.b')).to.not.be(undefined)
+      })
+
+      it('should generate correct CSS', () => {
+        expect(sheet.toString()).to.be(stripIndent`
+        .a-id {
+          float: left;
+        }
+        .a-id {
+          color: blue;
+        }
+        .a-id {
+          width: 0;
+        }
+        .a-id.b {
+          z-index: 1;
+        }
+        .a-id.b {
+          top: 0;
+        }
+      `)
+      })
+    })
+  })
+
   describe('.addRules()', () => {
     let sheet