sketch-hq/sketch-assistant-core-rules

Style equality method

Closed this issue · 5 comments

Objective

Move the style comparison methods into the rule utils object. These methods can keep the existing hash-based approach. These methods must cover our existing style comparison cases.

Motivation

The style comparison is being hand-rolled and repeated in a couple of rules:

  • layer-styles-no-dirty
  • layer-styles-prefer-shared
  • text-styles-no-dirty
  • text-styles-prefer-shared
  • 2 of the 3 library rules

It differs only slightly between them (e.g. text comparisons take into consideration the textStyle attribute).

This poses an opportunity to refactor these parts of the code and perhaps even speed them up.

Ideas

  1. Make it configurable (allowing to specify which attributes to use for the comparison)
  2. Avoid doing the hash and just convert it to a string (the argument here would be that strings are immutable and very fast to compare in JS - similar to number comparison - and object to string conversion is also very fast in JS through the JSON methods)
  3. Would we benefit from having a set of semantic comparison functions? (e.g. would it be cool for us to be able to tell equality between things like #000000/#000/RGB(0,0,0)` ?)

Approach in use

As a reference, the approach we are using sets the style attributes to be compared in a new object and reduces that object into a hash code. Using this technique we can then produce another hash code for the other style object, and compare these hashes to check for equality.

        // Get an md5 hash of the style object. Only consider a subset of style
        // object properties when computing the hash (can revisit this to make the
        // check looser or stricter)
        const hash = utils.objectHash({
          borders: layer.style?.borders,
          borderOptions: layer.style?.borderOptions,
          blur: layer.style?.blur,
          fills: layer.style?.fills,
          shadows: layer.style?.shadows,
          innerShadows: layer.style?.innerShadows,
        })

@HugoDaniel

Avoid doing the hash and just convert it to a string (the argument here would be that strings are immutable and very fast to compare in JS - similar to number comparison - and object to string conversion is also very fast in JS through the JSON methods)

Sounds interesting. Could you provide some code or a bit more detail how this might work?

I dont know, but thought about the lazy approach to object comparison in JS:

JSON.stringify(obj1) === JSON.stringify(obj2)

which normally triggers inequality if the attributes are specified in a different order:

let o1 = { foo: 'bar', baz: 'quz' };
let o2 = { baz: 'quz', foo: 'bar' };
JSON.stringify(o1) === JSON.stringify(o2); // false

we could control it though, by iterating through the attributes of one object while comparing them to the attributes of the other object, to enforce the same order of attributes.

What do you think of something along the lines of:

function styleEq(style1, style2, attributes = ['fills', 'borders', 'shadows',...]) {
    const subStyle1 = {}
    const subStyle2 = {}
    // filter the attributes into the substyles
    for (attrib of attributes) {
      subStyle1[attrib] = style1[attrib]
      subStyle2[attrib] = style2[attrib]
    }
    // compare them
    return JSON.stringify(subStyle1) === JSON.stringify(subStyle2);
}

Another possibility is that we try to do some sort of deep equality comparison (here is a poor man recursive object/array deep equality [ab]using object/array iterators and similar typeof value):

function deepObjEq(obj1, obj2, aux = true) {
   if (JSON.stringify([...Object.keys(obj1)].sort()) !== JSON.stringify([...Object.keys(obj2)].sort())) return false;
   for (let [k, v] of Object.entries(obj1)) {
      if (!aux) return false;
      if (typeof v !== 'object') {
        aux = v === obj2[k];
      } else {
        aux = deepObjEq(v, obj2[k])
      }
   }
   return aux;
}

or: just keep our working hash approach and move it to the utils package 😄 - it works, it is readable and understandable, and best of all: its already done 🥳

I could imagine the approach you outlined in the styleEq example above would be really fast.

Perhaps we could do it in two steps? First create some styleEq methods to handle the use cases we've discovered, and put them on the rule utils object - these could use the existing hash based approach initially. Then we'd be free to change the implementations within the styleEq methods over time, without introducing breaking changes to rules ... ?

yes perfect 😄