Key injection functionality

[james-camilleri]

I'm not sure if this makes sense, or if it's something that can already be done with existing functionality, but I thought of this use case for the internal framework I'm currently constructing and thought I'd follow up on it.

The use case is as follows: I have a sub-map used to house a few properties for components, e.g. buttons. The interface has a default button, and primary/secondary/success/danger/etc variants. I'd like to reference the a base colour for these properties, and then inject/hot-load what that base colour would be in a loop, so I can generate variants of the button without replicating a bunch of properties. I'd imagine it would look something like this:

$map: (
  'base-colour': rgb(50, 50, 50),
  'properties': (
    'highlight': #base-colour ('lighten': 50%),
    'disabled: #base-colour ('darken': 50%)
  )
)

...

// Default highlight:
get-token($map, 'properties->highlight');

// Primary highlight:
get-token($map, 'properties->highlight', $inject: ('#base-colour': rgb(20, 50, 200)));

Again, I'm not sure if this is crazy, or there's an infinitely less complex way to do it with the existing functionality, or there's a specific reason it shouldn't be done, but I thought it might come in handy for this use case. I don't think it would be too difficult to implement, but I thought I'd float the idea and get some feedback about its utility before taking a screwdriver to the codebase.

[mirisuzanne]

This is interesting, but is it a big improvement over handling the injection yourself?

// merge your injection into the map…
get-token(map-merge($map, ('#base-colour': rgb(20, 50, 200))), 'properties->highlight');

// split out for readability, as much as you want…
$inject: ('#base-colour': rgb(20, 50, 200));
$merged: map-merge($map, $inject);
get-token($merged, 'properties->highlight');

I'm not sure that the extra syntax adds much to that existing solution?

[james-camilleri]

Well, no, this is definitely a doable solution for my use case I suppose. It's more the fact that you'd need to re-flatten the map every time you inject, which would be a bit of a performance drag which concerned me most I suppose. That being said, I can also see your point that the performance hit might not be worth the API addition.

[mirisuzanne]

Hm, I get that - but wonder: if map flattening is such a big performance hit, do we need a more generic way to mitigate that? We could… expose a way to explicitly flatten your maps up-front, so the parser never needs to? You could then merge flattened maps without any need to re-flatten?

That would make flattening optional, and user-controlled…

[james-camilleri]

We could look into just doing a recursive deep-search rather than flattening, I'm not sure what the performance trade-off would be though. A map-get operation should be O(1), so I'm not sure at what point the time it takes to flatten a map provides a tangible performance benefit over the time it takes to simply carry out a chain of successive map-gets. I could run some benchmarks. We'd still need to flatten and sort if the "legacy" silent failure mode is enabled, but otherwise there'd be no real need for it. Again though, I'd need to run some benchmarks to see where that line between recursive gets and flattening is.

Also I just realised that the example given is relatively trivial when the key to be injected is in the first layer, but merging something that's two or three nesting levels in might be a bit harder. Especially if the replaced key and its usages aren't all in the same "branch" of the nested map, so to speak, so you wouldn't be able to retrieve a subset of your map and inject into that. I'll look into that recursive get thing before I burden your framework with impossibly complex scenarios.

[mirisuzanne]

Yeah, we initially used a recursive lookup rather than flattening, and found some performance benefit: https://github.com/oddbird/accoutrement/pull/25 - but I'd be happy to revisit that with more precise benchmarks.

[mirisuzanne]

Closing this as map-flattening shouldn't be needed in the Sass modules update. If we keep the parser, it will use Sass nested map functions.