Add support for deep-merging token maps

[johncrim]

Currently the accoutrement add-* mixins implement shallow-merging, which means that non-root values must be repeated. For example:

@use 'accoutrement/sass' as a;

// Issue: Accoutrement doesn't support deep-merging token maps
$default-colors: (
  'scrollbar': (
    'thumb': #555,
    'track': #ddd
  )
);
$custom-colors: (
  'scrollbar': (
    'thumb': #55a
  )
);

@include a.add-colors($default-colors, $custom-colors);
@debug a.color('scrollbar->track');

Results in :

$ sass shallow-merge.scss -I ./node_modules/
Error: ("thumb": #55a) isn't a valid CSS value.
   ╷
75 │     $new: if((type-of($new) == 'string') or (not $new), $new, '#{$new}');
   │                                                                  ^^^^
   ╵
  node_modules\accoutrement\sass\core\_strings.scss 75:66      -a-str-replace()
  node_modules\accoutrement\sass\core\_parser.scss 143:18      -a-replace()
  node_modules\accoutrement\sass\core\_parser.scss 102:14      -a-parse()
  node_modules\accoutrement\sass\core\_parser.scss 45:11       -a-get()
  node_modules\accoutrement\sass\core\_api.scss 305:9          get-token()
  node_modules\accoutrement\sass\plugin\color\_api.scss 56:11  color()
  shallow-merge.scss 18:8                                      root stylesheet

The error is related to #49 (which I'll still try to provide a PR for), but the root issue here is that deep-merging of maps isn't implemented. In other words the values that I'd like to inherit from default maps need to be repeated.

I have a workaround, which is to use a map-deep-merge function I wrote:

a.$colors: sass-util.map-deep-merge(a.$colors, $default-colors, $custom-colors);

I'm volunteering to add this to the accoutrement code base if you think it's useful. In case you think that it's important to preserve the current behavior, I'm thinking that something like an "accoutrement behavior" map would work, that defaults to current behavior. Eg:

@include a.set-behavior(('deep-merge': true));

I also think this concept of a global behavior map would be a good way to return null for missing keys without breaking current behavior, as discussed in #49. What do you think?

[mirisuzanne]

Yeah, this is a great idea.

(Also reminds me we should move to the new syntax soon… I'll open a story for that)

[johncrim]

Thanks @mirisuzanne. To clarify which parts you like:

• Do you think it's important that the current shallow merge behavior continue to be the default?
• Do you like the idea of a default behavior map (for this and returning null)?

[mirisuzanne]

• We used to provide advanced map functions in the public API, and it might be good to bring some of them back. There's an existing syntax proposal for Sass core that we could use as a basis (or make a PR against Sass, if you write Dart…).
• Yes: moving to deep-merge default is a good idea, but will require some deprecation, major release, etc… Could do it in two steps…
• Yes: a settings/behavior map would allow customizing several aspects. If we do that, it should be part of the core, but easily extended by plugins.

Caveat: Nested maps always cause a performance hit. We need to proceed carefully, to make sure we're not letting compile times get out of control.

Next Steps: I'll create a sass-modules fork of the master branch, and then we can do major-release features against that branch, while incrementally migrating the old features to module syntax as well. (I tried simply running the migration tool, but our partial-dependency web is too complex for it… May work better if we take it one module at a time.)

[mirisuzanne]

I submitted a formal proposal to sass/sass#2836 - tho the current syntax discussed doesn't do a full deep-merge. I'll push on that a bit. I expect performance will be much better if it happens in the compiler.

[mirisuzanne]

This has been implemented in Dart Sass. We should put that to use in a future release built on the latest Sass updates, but I think this issue can be closed.

[johncrim]

Hi @mirisuzanne - I'm in the process of migrating our codebase to the accoutrement 4.0.0 beta2, and I'm very happy with the changes/improvements so far - very nice work! Thank you!! I particularly like the null default for missing tokens, and in general while there are a number of breaking changes, I think this puts the project on great footing for new projects and new users adopting it.

I noticed that deep-merging token maps has either not been implemented or is not working. I know things have changed a lot since I filed this issue - lmk if you'd like to reopen this issue, or shall I provide a new issue? I'm also happy to provide a fix + tests.

For example, if I add this test to test/utils/_maps.scss in the modules branch, it fails:

    @include it('Deep merges maps') {
      $first: (
        'link': (
          'height': 3px,
          'offset': 0
        )
      );
      $second: (
        'link': (
          'offset': '#link->height' ('*': -1)
        )
      );
      $result: (
        'link': (
          'height': 3px,
          'offset': '#link->height' ('*': -1)
        )
      );

      @include assert-equal(map.multi-merge($first, $second), $result);
    }

The output is:

  455 passing (477ms)
  1 failing

  1) utils/map
       Multi-Merge [function]
         Deep merges maps:
     AssertionError [ERR_ASSERTION]: Deep merges maps ("[map] ("link": ("offset": "#link->height" ("*": -1)))" assert-equal "[map] ("link": ("height": 3px, "offset": "#link->height" ("*": -1)))")
     + expected - actual

     -[map] ("link": ("offset": "#link->height" ("*": -1)))
     +[map] ("link": ("height": 3px, "offset": "#link->height" ("*": -1)))

      at C:\src\github\css\accoutrement\node_modules\sass-true\lib\main.js:84:20
      at arrayEach (node_modules\lodash\lodash.js:530:11)
      at forEach (node_modules\lodash\lodash.js:9410:14)
      at Context.<anonymous> (node_modules\sass-true\lib\main.js:82:9)
      at processImmediate (node:internal/timers:464:21)

As I noted before, if you think the shallow merge behavior is important to preserve, we could; and could add a new API for each of the modules for adding tokens with deep merge behavior (eg scale.merge-sizes). In my opinion the deep merge behavior is preferable - I think it's what most people would expect, instead of just merging root tokens and overwriting their sub-trees.

The only advantage I see for shallow merging is that it provides sub-tree replacement, which could be useful if you want to remove values, since there is no other mechanism for removal. But in general I don't think that's something most people would need.

I don't even think it would be much of a breaking change to change add-* to deep merge - the change would be that tokens in earlier maps that weren't present in the output would show up. Since the default missing key behavior was difficult to work with before, I think it's likely that very few missing key issues would be impacted by this change.

[mirisuzanne]

Thanks for catching this, and submitting a patch! I'll take a look as soon as I can.

[mirisuzanne]

merged the PR, thanks!