Updating styles a list item which has a nested rule (#1242)

* refactor createUseStyles * add a test for list item update * fix addRule() support for non-unique rule names * update snapshotst * fix typo * Update changelog.md Co-authored-by: Bono Stebler <bs85@users.noreply.github.com>
cssinjs · Dec 28, 2019 · 3a536de · 3a536de
1 parent 9f7924e
commit 3a536de
Show file tree

Hide file tree

Showing 21 changed files with 316 additions and 148 deletions.
diff --git a/changelog.md b/changelog.md
@@ -7,6 +7,8 @@ Since you are interested in what happens next, in case, you work for a for-profi
 ### Bug fixes
 
 - [jss-plugin-vendor-prefixer] Upgrade css-vendor package to v2.0.7 ([1208](https://github.com/cssinjs/jss/pull/1208))
+- [jss] Fix `sheet.addRule()` support for duplicate rule names ([1242](https://github.com/cssinjs/jss/pull/1242))
+  [react-jss] Fix function values support inside of nested media queries when component is a list item ([1242](https://github.com/cssinjs/jss/pull/1242))
 
 ## 10.0.0 (2019-9-22)
 

diff --git a/packages/css-jss/.size-snapshot.json b/packages/css-jss/.size-snapshot.json
@@ -1,13 +1,13 @@
 {
   "dist/css-jss.js": {
-    "bundled": 58303,
-    "minified": 20625,
-    "gzipped": 6967
+    "bundled": 58440,
+    "minified": 20647,
+    "gzipped": 6976
   },
   "dist/css-jss.min.js": {
-    "bundled": 57541,
-    "minified": 20163,
-    "gzipped": 6744
+    "bundled": 57678,
+    "minified": 20185,
+    "gzipped": 6756
   },
   "dist/css-jss.cjs.js": {
     "bundled": 2919,

diff --git a/packages/jss-plugin-nested/.size-snapshot.json b/packages/jss-plugin-nested/.size-snapshot.json
@@ -1,23 +1,23 @@
 {
   "dist/jss-plugin-nested.js": {
-    "bundled": 4520,
-    "minified": 1556,
-    "gzipped": 857
+    "bundled": 4657,
+    "minified": 1578,
+    "gzipped": 869
   },
   "dist/jss-plugin-nested.min.js": {
-    "bundled": 4082,
-    "minified": 1340,
-    "gzipped": 726
+    "bundled": 4219,
+    "minified": 1362,
+    "gzipped": 737
   },
   "dist/jss-plugin-nested.cjs.js": {
-    "bundled": 3651,
-    "minified": 1416,
-    "gzipped": 768
+    "bundled": 3784,
+    "minified": 1438,
+    "gzipped": 777
   },
   "dist/jss-plugin-nested.esm.js": {
-    "bundled": 3417,
-    "minified": 1237,
-    "gzipped": 685,
+    "bundled": 3550,
+    "minified": 1259,
+    "gzipped": 696,
     "treeshaked": {
       "rollup": {
         "code": 64,

diff --git a/packages/jss-plugin-nested/src/index.js b/packages/jss-plugin-nested/src/index.js
@@ -53,18 +53,21 @@ export default function jssNested(): Plugin {
     return result
   }
 
-  function getOptions(rule, container, options) {
+  function getOptions(rule, container, prevOptions) {
     // Options has been already created, now we only increase index.
-    if (options) return {...options, index: options.index + 1}
+    if (prevOptions) return {...prevOptions, index: prevOptions.index + 1}
 
     let {nestingLevel} = rule.options
     nestingLevel = nestingLevel === undefined ? 1 : nestingLevel + 1
 
-    return {
+    const options = {
       ...rule.options,
       nestingLevel,
       index: container.indexOf(rule) + 1
     }
+    // We don't need the parent name to be set options for chlid.
+    delete options.name
+    return options
   }
 
   function onProcessStyle(style, rule, sheet?: StyleSheet) {

diff --git a/packages/jss-preset-default/.size-snapshot.json b/packages/jss-preset-default/.size-snapshot.json
@@ -1,13 +1,13 @@
 {
   "dist/jss-preset-default.js": {
-    "bundled": 55547,
-    "minified": 19859,
-    "gzipped": 6612
+    "bundled": 55684,
+    "minified": 19881,
+    "gzipped": 6622
   },
   "dist/jss-preset-default.min.js": {
-    "bundled": 54785,
-    "minified": 19397,
-    "gzipped": 6395
+    "bundled": 54922,
+    "minified": 19419,
+    "gzipped": 6405
   },
   "dist/jss-preset-default.cjs.js": {
     "bundled": 1329,

diff --git a/packages/jss-starter-kit/.size-snapshot.json b/packages/jss-starter-kit/.size-snapshot.json
@@ -1,13 +1,13 @@
 {
   "dist/jss-starter-kit.js": {
-    "bundled": 71089,
-    "minified": 29899,
-    "gzipped": 9233
+    "bundled": 71226,
+    "minified": 29921,
+    "gzipped": 9245
   },
   "dist/jss-starter-kit.min.js": {
-    "bundled": 70327,
-    "minified": 29437,
-    "gzipped": 9019
+    "bundled": 70464,
+    "minified": 29459,
+    "gzipped": 9031
   },
   "dist/jss-starter-kit.cjs.js": {
     "bundled": 2592,

diff --git a/packages/jss/.size-snapshot.json b/packages/jss/.size-snapshot.json
@@ -1,30 +1,30 @@
 {
   "dist/jss.js": {
-    "bundled": 60970,
-    "minified": 22613,
-    "gzipped": 6824
+    "bundled": 61595,
+    "minified": 22819,
+    "gzipped": 6882
   },
   "dist/jss.min.js": {
-    "bundled": 59593,
-    "minified": 21844,
-    "gzipped": 6463
+    "bundled": 60218,
+    "minified": 22050,
+    "gzipped": 6522
   },
   "dist/jss.cjs.js": {
-    "bundled": 55751,
-    "minified": 24513,
-    "gzipped": 6813
+    "bundled": 56362,
+    "minified": 24719,
+    "gzipped": 6879
   },
   "dist/jss.esm.js": {
-    "bundled": 55219,
-    "minified": 24078,
-    "gzipped": 6724,
+    "bundled": 55830,
+    "minified": 24284,
+    "gzipped": 6787,
     "treeshaked": {
       "rollup": {
-        "code": 19841,
+        "code": 20047,
         "import_statements": 352
       },
       "webpack": {
-        "code": 21308
+        "code": 21514
       }
     }
   }

diff --git a/packages/jss/src/Jss.js b/packages/jss/src/Jss.js
@@ -108,14 +108,15 @@ export default class Jss {
 
   /**
    * Create a rule without a Style Sheet.
+   * [Deprecated] will be removed in the next major version.
    */
   createRule(name?: string, style?: JssStyle = {}, options?: RuleFactoryOptions = {}): Rule | null {
     // Enable rule without name for inline styles.
     if (typeof name === 'object') {
       return this.createRule(undefined, name, style)
     }
 
-    const ruleOptions: RuleOptions = {...options, jss: this, Renderer: this.options.Renderer}
+    const ruleOptions: RuleOptions = {...options, name, jss: this, Renderer: this.options.Renderer}
 
     if (!ruleOptions.generateId) ruleOptions.generateId = this.generateId
     if (!ruleOptions.classes) ruleOptions.classes = {}

diff --git a/packages/jss/src/RuleList.js b/packages/jss/src/RuleList.js
@@ -37,6 +37,8 @@ export default class RuleList {
   // Used to ensure correct rules order.
   index: Array<Rule> = []
 
+  counter: number = 0
+
   options: RuleListOptions
 
   classes: Classes
@@ -54,7 +56,7 @@ export default class RuleList {
    *
    * Will not render after Style Sheet was rendered the first time.
    */
-  add(key: string, decl: JssStyle, ruleOptions?: RuleOptions): Rule | null {
+  add(name: string, decl: JssStyle, ruleOptions?: RuleOptions): Rule | null {
     const {parent, sheet, jss, Renderer, generateId, scoped} = this.options
     const options = {
       classes: this.classes,
@@ -64,15 +66,24 @@ export default class RuleList {
       Renderer,
       generateId,
       scoped,
+      name,
       ...ruleOptions
     }
 
+    // When user uses .createStyleSheet(), duplicate names are not possible, but
+    // `sheet.addRule()` opens the door for any duplicate rule name. When this happens
+    // we need to make the key unique within this RuleList instance scope.
+    let key = name
+    if (name in this.raw) {
+      key = `${name}-d${this.counter++}`
+    }
+
     // We need to save the original decl before creating the rule
     // because cache plugin needs to use it as a key to return a cached rule.
     this.raw[key] = decl
 
     if (key in this.classes) {
-      // For e.g. rules inside of @media container
+      // E.g. rules inside of @media container
       options.selector = `.${escape(this.classes[key])}`
     }
 
@@ -101,7 +112,7 @@ export default class RuleList {
   remove(rule: Rule): void {
     this.unregister(rule)
     delete this.raw[rule.key]
-    this.index.splice(this.indexOf(rule), 1)
+    this.index.splice(this.index.indexOf(rule), 1)
   }
 
   /**
@@ -122,7 +133,7 @@ export default class RuleList {
   }
 
   /**
-   * Register a rule in `.map` and `.classes` maps.
+   * Register a rule in `.map`, `.classes` and `.keyframes` maps.
    */
   register(rule: Rule): void {
     this.map[rule.key] = rule
@@ -169,18 +180,18 @@ export default class RuleList {
     }
 
     if (name) {
-      this.onUpdate(data, this.get(name), options)
+      this.updateOne(this.map[name], data, options)
     } else {
       for (let index = 0; index < this.index.length; index++) {
-        this.onUpdate(data, this.index[index], options)
+        this.updateOne(this.index[index], data, options)
       }
     }
   }
 
   /**
    * Execute plugins, update rule props.
    */
-  onUpdate(data: Object, rule: Rule, options?: Object = defaultUpdateOptions) {
+  updateOne(rule: Rule, data: Object, options?: Object = defaultUpdateOptions) {
     const {
       jss: {plugins},
       sheet

diff --git a/packages/jss/src/StyleSheet.js b/packages/jss/src/StyleSheet.js
@@ -6,7 +6,6 @@ import type {
   ToCssOptions,
   RuleOptions,
   StyleSheetOptions,
-  UpdateArguments,
   JssStyle,
   Classes,
   KeyframesMap,
@@ -31,6 +30,10 @@ export default class StyleSheet {
 
   queue: ?Array<Rule>
 
+  update: typeof RuleList.prototype.update
+
+  updateOne: typeof RuleList.prototype.updateOne
+
   constructor(styles: JssStyles, options: StyleSheetOptions) {
     this.attached = false
     this.deployed = false
@@ -47,6 +50,8 @@ export default class StyleSheet {
       this.renderer = new options.Renderer(this)
     }
     this.rules = new RuleList(this.options)
+    this.update = this.rules.update.bind(this.rules)
+    this.updateOne = this.rules.updateOne.bind(this.rules)
 
     for (const name in styles) {
       this.rules.add(name, styles[name])
@@ -150,8 +155,8 @@ export default class StyleSheet {
    * Delete a rule by name.
    * Returns `true`: if rule has been deleted from the DOM.
    */
-  deleteRule(name: string): boolean {
-    const rule = this.rules.get(name)
+  deleteRule(name: string | Rule): boolean {
+    const rule = typeof name === 'object' ? name : this.rules.get(name)
 
     if (!rule) return false
 
@@ -180,14 +185,6 @@ export default class StyleSheet {
     return this
   }
 
-  /**
-   * Update the function values with a new data.
-   */
-  update(...args: UpdateArguments): this {
-    this.rules.update(...args)
-    return this
-  }
-
   /**
    * Convert rules to a CSS string.
    */

diff --git a/packages/jss/src/plugins/conditionalRule.js b/packages/jss/src/plugins/conditionalRule.js
@@ -19,6 +19,8 @@ export class ConditionalRule implements ContainerRule {
 
   key: string
 
+  query: string
+
   rules: RuleList
 
   options: RuleOptions
@@ -29,6 +31,8 @@ export class ConditionalRule implements ContainerRule {
 
   constructor(key: string, styles: Object, options: RuleOptions) {
     this.key = key
+    // Key might contain a unique suffix in case the `name` passed by user was duplicate.
+    this.query = options.name
     const atMatch = key.match(atRegExp)
     this.at = atMatch ? atMatch[1] : 'unknown'
     this.options = options
@@ -72,10 +76,10 @@ export class ConditionalRule implements ContainerRule {
     if (options.indent == null) options.indent = defaultToStringOptions.indent
     if (options.children == null) options.children = defaultToStringOptions.children
     if (options.children === false) {
-      return `${this.key} {}`
+      return `${this.query} {}`
     }
     const children = this.rules.toString(options)
-    return children ? `${this.key} {\n${children}\n}` : ''
+    return children ? `${this.query} {\n${children}\n}` : ''
   }
 }
 

diff --git a/packages/jss/src/types/jss.js b/packages/jss/src/types/jss.js
@@ -118,7 +118,8 @@ export type RuleOptions = {
   keyframes: KeyframesMap,
   jss: Jss,
   generateId: GenerateId,
-  Renderer?: Class<Renderer> | null
+  Renderer?: Class<Renderer> | null,
+  name: string
 }
 
 export type RuleListOptions = {

diff --git a/packages/jss/tests/integration/sheet.js b/packages/jss/tests/integration/sheet.js
@@ -121,6 +121,24 @@ describe('Integration: sheet', () => {
         }
       `)
     })
+
+    it('should add rules with duplicate names', () => {
+      const sheet = jss.createStyleSheet()
+      sheet.addRule('a', {color: 'red'})
+      sheet.addRule('a', {color: 'green'})
+      expect(sheet.classes).to.eql({
+        a: 'a-id',
+        'a-d0': 'a-d0-id'
+      })
+      expect(sheet.toString()).to.be(stripIndent`
+        .a-id {
+          color: red;
+        }
+        .a-d0-id {
+          color: green;
+        }
+      `)
+    })
   })
 
   describe('sheet.deleteRule()', () => {