Stop pruning in nestedProperty?

[alexcjohnson]

Lib.nestedProperty started as a nice compact, decoupled way to manage edits to traces and layout objects. We have a system that prunes empty containers, which was originally meant so that unnecessary pieces would not be left around after multiple operations. For example, you have a line trace with default properties:

{x: [1, 2, 3], y: [1, 2, 3]}

then you edit the line color restyle(gd, {'line.color': 'red'})

{x: [1, 2, 3], y: [1, 2, 3], line: {color: 'red'}}

And you don't like that, so you undo it restyle(gd, {'line.color': null}). Without pruning you get:

{x: [1, 2, 3], y: [1, 2, 3], line: {}}

Which is functionally the same, but it's not a perfect undo.

However, some empty containers cannot be removed (see #1403 (comment) and #1403 (comment) ) so this has gotten too coupled to our particular JSON structure. Can we live without doing any pruning since it would simplify the logic so much? @etpinard and I are leaning toward doing that but wanted to invite discussion.

[rreusser]

I can imagine it would work, but is it too ugly to use a flag in the plot schema to prevent pruning but allow it by default?

[rreusser]

Ohhh, never mind. Yeah, that's the coupling obviously. Then my vote is to allow pruning, or at least move pruning to a dedicated step that is aware of the plot schema and perhaps not run so frequently.

[alexcjohnson]

Ohhh, never mind. Yeah, that's the coupling obviously.

and worse, nestedProperty isn't always called with a trace or complete layout as the root object.

[rreusser]

Yeah, I think it's excellent to keep it fully decoupled and (as) unaware of the schema (as possible). It seems to me like pruning is both a perfectly nice/valid thing and also something that shouldn't be related to nested properties.

[etpinard]

cc @bpostlethwaite and @VeraZab can you think of any potential problems that stopping pruning empty containers could cause in W2?

[VeraZab]

I'm not sure I understand what the situation would look like without pruning..
So instead of calling restyle(gd, {'line.color': null}) I would call restyle(gd, line: {}) ?

Of the top of my head, I can think of one situation where we use nulling:
when I want to remove a shape or an image, I call relayout(gd, {'images[2]': null})
How would I do this now?

[alexcjohnson]

@VeraZab I don't think anything would change in how you call restyle or relayout - the only difference is that occasionally you'd end up with an empty container (like line: {}) where now we notice the empty container and delete it in the pruning step. It doesn't seem to me like the workspace would be bothered by these containers lingering, but @etpinard is absolutely right to check on it.

Calls like relayout(gd, {'images[2]': null}) still hit logic to turn that into splicing the item out of the array - and #1403 standardizes that logic to work across all the container arrays in layout.

[VeraZab]

@alexcjohnson thanks for the clarification.
Alright, well I don't see any blockers on this for the workspace.

[alexcjohnson]

Update: I notice one case that {} is different from undefined, and that's rangeslider, eg:

Plotly.newPlot(gd,[{x:[1,2,3]}],{xaxis:{rangeslider: {}}})

Changing that will probably have to wait for v2... interestingly once you have a rangeslider we add some things to it (in the input layout copy) so the bug I was expecting (eg Plotly.relayout(gd, {'xaxis.title': null}) accidentally deleting the rangeslider) is not manifest, but there is a related bug: you can't add a rangeslider by Plotly.relayout(gd, {'xaxis.rangeslider': {}}) because nestedProperty sees {} and prunes it.

Anyway, unless something has changed in the 14 months 🙄 that this issue has lain dormant, seems like we're agreed that we can 🔪 the pruning check. I need to remove it at least within supplyDefaults - where it's always been useless (and wasteful 🐎) but now is actually causing me other bugs while I try to fix #2508. Would be easier to just remove than to make a special version to use in supplyDefaults

[etpinard]

More on the range slider topic. Here's one weird test:

plotly.js/test/jasmine/tests/range_slider_test.js

Lines 376 to 387 in 4ed586a

	it('should add the slider if rangeslider is set to anything', function(done) {
	Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
	.then(function() {
	return Plotly.relayout(gd, 'xaxis.rangeslider', 'exists');
	})
	.then(function() {
	var rangeSlider = getRangeSlider();
	expect(rangeSlider).toBeDefined();
	})
	.catch(failTest)
	.then(done);
	});

that uses relayout(gd, 'xaxis.rangeslider', 'exists') to add a range slider. Tests

plotly.js/test/jasmine/tests/range_slider_test.js

Lines 389 to 422 in 4ed586a

	it('should add the slider if visible changed to `true`', function(done) {
	Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
	.then(function() {
	return Plotly.relayout(gd, 'xaxis.rangeslider.visible', true);
	})
	.then(function() {
	var rangeSlider = getRangeSlider();
	expect(rangeSlider).toBeDefined();
	expect(countRangeSliderClipPaths()).toEqual(1);
	})
	.catch(failTest)
	.then(done);
	});
	it('should remove the slider if changed to `false` or `undefined`', function(done) {
	Plotly.plot(gd, [{
	x: [1, 2, 3],
	y: [2, 3, 4]
	}], {
	xaxis: {
	rangeslider: { visible: true }
	}
	})
	.then(function() {
	return Plotly.relayout(gd, 'xaxis.rangeslider.visible', false);
	})
	.then(function() {
	var rangeSlider = getRangeSlider();
	expect(rangeSlider).not.toBeDefined();
	expect(countRangeSliderClipPaths()).toEqual(0);
	})
	.catch(failTest)
	.then(done);
	});

are a little aligned with the rest of our components.

[etpinard]

seems like we're agreed that we can hocho the pruning check. I need to remove it at least within supplyDefaults - where it's always been useless (and wasteful racehorse) but now is actually causing me other bugs while I try to fix #2508. Would be easier to just remove than to make a special version to use in supplyDefaults

As I wrote in a private convo yesterday, 👍 for 🔪ing the prune check during supplyDefaults. Today, I would even go one step further: the only place where pruning empty containers makes sense (and even that's debatable) is during restyle/relayout/update.

[alexcjohnson]

the only place where pruning empty containers makes sense (and even that's debatable) is during restyle/relayout/update.

Right, I guess I'm arguing for not even pruning {} during restyle/relayout/update - the only place this could have a practical effect (rangeslider) it doesn't anyway due to our clandestine mutations.

In fact... currently we also prune [] with exceptions for data arrays, which we determine in the negative by matching the attribute string against a hard-coded collection of known info_array names

plotly.js/src/lib/nested_property.js

Line 138 in 4ed586a

var INFO_PATTERNS = /(^|\.)((domain|range)(\.[xy])?|args|parallels)$/;

yet there too we have a note:

 * Info arrays can be safely deleted, but not deleting them has no ill effects other
 * than leaving a trace or layout object with some cruft in it.

Honestly this has probably never actually happened to a user - using restyle etc to remove all items from an info_array like range or domain, and expecting the array container to disappear? So as much as it was a nice idea that a restyle and then its undo operation should put the object back into its original state exactly, the fact is with no pruning we get the same practical result with far less complexity and coupling. I've come around to thinking we shouldn't treat [] as something to delete either - and I'm going to do that now, along with 🔪 the rest of the pruning stuff, unless anyone objects.

[etpinard]

I'm big fan of making nestedProperty less coupled. So 👍 for all things listed in the above comment ⤴️

As a side-note, making nestedProperty less coupled should make it easier to publish as a standalone module (if ever we choose that path) -> #2138