sketch-hq / sketch-assistants

Monorepo containing the official Sketch Assistants, along with utility functions and types for Sketch Assistant development.
MIT License
45 stars 11 forks source link

Change config merge behaviour for extended Assistants #188

Closed christianklotz closed 3 years ago

christianklotz commented 3 years ago

Currently, extending an Assistant will merge any new Assistant configuration with the configuration provided by the base Assistant. However, any existing rule configs are replaced, not merged, by newly provided rule configs.

Given an original configuration such as:

{
  rules: {
    'rule-1': {
      active: true,
    },
    'rule-2': {
      active: true,
      foo: 1,
    },
    'rule-3': {
      active: true,
      bar: "hello",
      baz: "world",
    },
  }
}

… extending the Assistant with the following config:

{
  rules: {
    'rule-1': {
      active: false,
    },
    'rule-3': {
      active: true,
      baz: "everyone",
    },
}

… will result in:

{
  rules: {
    'rule-2': {
      active: true,
      foo: 1,
    },
    'rule-3': {
      active: true,
      baz: "world",
    },
  }
}

This configuration is invalid because rule-3 does not include all required properties with bar missing. One option is to always provide a full configuration for each rule but this seems inconsistent given that the Assistant configuration overall can be partial.

Instead of shallow merging rules, it would be better to deep merge them as it makes it easier to tweak existing Assistants.

jedrichards commented 3 years ago

I think your final code block above should be

{
  rules: {
    'rule-1': {
      active: false,
    },
    'rule-2': {
      active: true,
      foo: 1,
    },
    'rule-3': {
      active: true,
      baz: "everyone",
    },
  }
}
christianklotz commented 3 years ago

Right, indeed … I omitted the one with the active: false. Your version is the desired one.