Fractal: Variants' contexts get extended from the default context.

[matijs]

Variant contexts seem to be created by extending the default context. Consider the example below.

---
title: foo
context:
  options:
    - one
    - two
    - three
    - four
variants:
  -
    name: bar
    context:
      options:
        - alpha
        - beta

The options variable in the bar variant's context will contain ['alpha', 'beta', 'three', 'four'] which is not what I'd expect when setting options explicitly in the variant's own context. A workaround for now is to not define a context for the default version of a component.

[allmarkedup]

Hi @matijs that is actually the expected behaviour. The top-level context object is inherited and merged into all variants. Although the array behaviour does seem to trip people up so it may be worth revisiting...!

You can set context data for the default variant _only_ by explicitly defining a variant with the name default (this normally happens behind-the-scenes) and then giving it it's own data, something like:

---
title: foo
context:
  title: This is inherited unless overriden in a variant
variants:
-
  name: default
  context:
    options:
      - foo
      - bar
  -
    name: bar
    context:
      title: this overrides the default title
      options:
        - alpha
        - beta

[matijs]

@allmarkedup the array behaviour was indeed what tripped me up. I did expect the merging, just not this deep :)

name: default
label: not default 

Solves this (and more) though! Thanks.

[allmarkedup]

Ok great. I think that the array behaviour (even though it was what I originally intended) is so counter-intuitive (based on people's feedback anyway!) that it should actually maybe considered a bug. I may well change it in a future release.

[allmarkedup]

I've reviewed this behaviour and I'm re-opening this issue because I think the array merging behaviour should be considered a bug. The context object merging is intended but arrays should not be merged as it is definitely not what would be expected.

[felixdorner]

I don't know if this comment fits here but it might be a point to consider:

When I try to place a component-A--variant in component-B the content set in its config gets replaced by the default content of component-A (same config). I know that I could use ‘render’ instead of ‘>’ to place components and keep the content but in my case, I’d like to change something minor and keep the content of component-A--variant while placing it in component-B without it falling back to the default content of component-A.

I hope my description is understandable. For me, the expected behavior is not met here.

[Chapabu]

@LeBenLeBen I'm a bit worried about this one now. It's been around long enough that I wonder if it's become a feature, rather than a bug; and fixing it will break a _lot_ if Fractal installs. It feels like fixing this is a major enough change to warrant a major version bump unless we ignore semver :(

[marchdoe]

Count me in the feature camp, I thought this was intended.

[LeBenLeBen]

The first time I encountered this behavior I thought it was a bug and I always worked around it by redefining the whole context instead. I don't really see how you could benefit from it looking at how bad the array combination results usually are. But who am I to know how people used it 😄

So yeah, major bump it has to be. I guess making a major release of both Handlebars and Nunjucks adapters would not be an issue. But then nothing forces us to bump Handlebars within the Fractal 1.x repository, which would not break existing installs while still allowing people to manually update it to benefit from the changes.

[Chapabu]

That sounds like a great idea @LeBenLeBen. We can just document it on the adapter README files, and leave it as an "opt-in" thing.

[allmarkedup]

@Chapabu @LeBenLeBen hey guys, I'm just trying to get my head around this issue again. I believe that this specific issue was actually resolved with the 1.1 release? I.e. arrays in the 'default' component context are now replaced (rather than merged) with the corresponding array (if defined) in variant context.

In this example:

module.exports = {
  context: {
    numbers: ["one", "two", "three", "four"]
  },
  variants: [
    {
      name: "testing",
      context: {
        numbers: ["five", "six"]
      }
    }
  ]
};

The numbers array for the variant testing prior to 1.1 would have had the value ["five", "six", "three", "four"]. Post 1.1 it has the value ["five","six"] which (for me) is the expected result - the array has been replaced and not merged.

If you agree, then we can close this and then separately fix the (still pre-1.1-style) merging behaviour with passed-in context data in the Handlebars and Nunjucks adapters, and bump them to the next major release as suggested.

Does that sound good to you guys?

[allmarkedup]

One thing to be considered with the handlebars adapter version bump - as it's bundled as the default with Fractal and the merged PR here would be a breaking change, the 'default' install of Fractal would have to still use the 1.x handlebars adapter version.

Users would have to manually install and register the latest ('fixed') version. A bit of a pain but otherwise it might end up breaking a lot of libraries that are relying on this unexpected behaviour.

[LeBenLeBen]

@allmarkedup The handlebars issue as been merged but not released and as you said, it should be a major version bump and not replicated in the existing Fractal repo without a major bump as well (cf. my comment above).

The Nunjucks issue is still open though and it would be geat if it could be merged and released as well so all adapters behave the same regarding context merging.

[allmarkedup]

Ah sorry @LeBenLeBen I somehow missed the whole second part of your comment above :-) I'll look at the Nunjucks PR today and then let's see if we can get 2.0 versions of both of those out ASAP..