vega / vega-embed

Publish Vega visualizations as embedded web components with interactive parameters.
BSD 3-Clause "New" or "Revised" License
392 stars 86 forks source link

how does `patch` work? #232

Closed g3o2 closed 5 years ago

g3o2 commented 5 years ago

You may interpret this issue as a question and/or a comment on the completeness of the documentation.

The patch example provided on observablehq shows how to add a property. However, is it possible to modify an already existing property in vega-embed 5 (e.g. changing the scale properties) ? In past versions of vega-embed, this was possible, with the constraint that one had to provide the complete object.

E.g. when you wanted to change a property of an already existing scale, you had to provide all scales as an array in the patch. With version 5, this behaviour no longer applies and I get a "Duplicate" error.

domoritz commented 5 years ago

Yeah, I changed the behavior of merging with objects so that you can add a signal easily. The safest and most robust option is to pass a function to patch that adds modifies whatever you want to modify. It's a bit tricky to get the patching right with just an object. Maybe we should use something https://tools.ietf.org/html/rfc6902 instead of a normal object.

g3o2 commented 5 years ago

If you want to fix a vega-lite spec as an end-user, passing an object (in the form of vega spec or some alternative specification that you suggest in your last sentence) makes probably more sense. It appears to me, with my limited knowledge of javascript, that passing a function as a patch is the natural solution when you are a developer.

I agree that the previous behaviour was not more practical than the current one - as already mentioned, changing a single scale would mean adding all scales to the patch object, which then exceeded the scope of a patch.

domoritz commented 5 years ago

Do you think I should switch to using https://www.npmjs.com/package/fast-json-patch and use a patch object instead of merging an object into the spec?

g3o2 commented 5 years ago

Looks interesting, though it adds a syntax dependency. If it was my decision to take, I would add the feature without breaking the "new old" behaviour. That is: accept both patch and vega objects.

domoritz commented 5 years ago

I think the old behavior is inconsistent and hard to reason about. Of course this will need a new major version so you can always get the old behavior using a previous version.

g3o2 commented 5 years ago

when I say new old, I'm referring to the current behaviour, which is quite easy: you can only add to the spec.

domoritz commented 5 years ago

You can overwrite as well (except an array with an array or an object with an object).

g3o2 commented 5 years ago

@domoritz thank you! interesting and promising the documentation to which you refer; I'll test this in the following days ...

gordonwoodhull commented 4 years ago

FWIW JSON Patch is great but it's not perfectly suited for Vega specs because there are a lot of arrays with named items. For example signals have a name: field, and JSON Patch doesn't have any way to match an array item by the value of one of its fields.

It would be unsafe to assume these arrays will be generated in the same order, since the input Vega-Lite spec will change, and there will be new versions of Vega-Lite.

I ended up creating a patcher function which takes input like

{
  "signals": {
    "selection1_x": [
      {
        "op": "replace",
        "path": "/on/0/update",
        "value": "[scale(\"x\", datetime(year(invert(\"x\", x(unit))), month(invert(\"x\", x(unit))))), scale(\"x\", datetime(year(invert(\"x\", x(unit))), month(invert(\"x\", x(unit)))))]"
      },
      {
        "op": "replace",
        "path": "/on/1/update",
        "value": "[selection1_x[0], scale(\"x\", datetime(year(invert(\"x\", clamp(x(unit),0,width))), month(invert(\"x\", clamp(x(unit),0,width)))))]"
      },
    ]
  }
}

and applies JSON patches within individual signals. (Even here I am assuming that the on event array is generated in a particular order, which could bite me in the future. If so I will probably have to make the patcher function recursive.)

This function works for signals, looks like it should work for scales, and it could work for other of these "named arrays" with some adaptation.

function patcher(patches) {
    return function(spec) {
        let spec2 = {...spec};
        Object.entries(patches).forEach(([key,value]) => {
            spec2[key] = spec[key].map(
                item => value[item.name] ?
                    applyPatch(item, value[item.name], true, false).newDocument :
                    item);
        });
        return spec2;
    };
}

Doubtless there are standard JSON searching and patching systems that go beyond JSON Patch, but I haven't looked into it yet.

domoritz commented 4 years ago

Thanks for your comment. I agree that json patch isn’t ideal but at least it’s a standard with well known limitations. We will continue to evaluate other options. I’m very open to changing Vega-Embed.