Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions draftlogs/7579_fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Update config diff check method to handle nested arrays [[#7579](https://github.com/plotly/plotly.js/pull/7579)]
38 changes: 35 additions & 3 deletions src/plot_api/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ function getParent(attr) {
if (tail > 0) return attr.substr(0, tail);
}

/*
/**
* hasParent: does an attribute object contain a parent of the given attribute?
* for example, given 'images[2].x' do we also have 'images' or 'images[2]'?
*
Expand All @@ -475,6 +475,7 @@ exports.hasParent = function (aobj, attr) {
return false;
};

const AX_LETTERS = ['x', 'y', 'z'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put at top of file perhaps? I would normally expect constants not inside a function to be defined at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do.

/**
* Empty out types for all axes containing these traces so we auto-set them again
*
Expand All @@ -483,12 +484,11 @@ exports.hasParent = function (aobj, attr) {
* @param {object} layoutUpdate: any update being done concurrently to the layout,
* which may supercede clearing the axis types
*/
var axLetters = ['x', 'y', 'z'];
exports.clearAxisTypes = function (gd, traces, layoutUpdate) {
for (var i = 0; i < traces.length; i++) {
var trace = gd._fullData[i];
for (var j = 0; j < 3; j++) {
var ax = getFromTrace(gd, trace, axLetters[j]);
var ax = getFromTrace(gd, trace, AX_LETTERS[j]);

// do not clear log type - that's never an auto result so must have been intentional
if (ax && ax.type !== 'log') {
Expand All @@ -507,3 +507,35 @@ exports.clearAxisTypes = function (gd, traces, layoutUpdate) {
}
}
};

/**
* Check if a collection (object or array) has changed given two versions of
* the collection: old and new.
*
* @param {Object|Array} oldCollection: Old version of collection to compare
* @param {Object|Array} newCollection: New version of collection to compare
*/
const hasCollectionChanged = (oldCollection, newCollection) => {
Copy link
Contributor

@emilykl emilykl Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit, but I would expect this function to be called something slightly more general like areObjectsEqual() since the function itself doesn't care whether oldCollection and newCollection are versions of the same object or not; that's a matter of how the function is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to be generic with the term "collection" since you could be passing in an object or an array, but we could go further as you suggest. Would areCollectionsEqual be acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, areCollectionsEqual (or maybe collectionsAreEqual?) sounds good to me 👍

Actually, one related question — what is the reason for ignoring keys starting with an underscore? Is that due to a JavaScript implementation detail, or a Plotly.js one? Maybe that should be captured in the function name (since it's a very particular definition of equality)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a plotly.js detail, but it's also a JavaScript convention. Variables that start with underscore are labeled as such because they are meant to be used internally by the library. So, you wouldn't see a user changing those values, hence they can be ignored.

const isArrayOrObject = (...vals) => vals.every((v) => Lib.isPlainObject(v)) || vals.every((v) => Array.isArray(v));
if ([oldCollection, newCollection].every((a) => Array.isArray(a))) {
if (oldCollection.length !== newCollection.length) return true;

for (let i = 0; i < oldCollection.length; i++) {
const oldVal = oldCollection[i];
const newVal = newCollection[i];
if (oldVal !== newVal) return isArrayOrObject(oldVal, newVal) ? hasCollectionChanged(oldVal, newVal) : true;
}
} else {
if (Object.keys(oldCollection).length !== Object.keys(newCollection).length) return true;

for (const k in oldCollection) {
if (k.startsWith('_')) return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems wrong, am I missing something? Is it supposed to be if (k.startsWith('_')) continue; ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, looking closely this whole loop is problematic; it should only ever return true from within the loop. Line 535 will return false sometimes and short-circuit the whole loop if hasCollectionChanged(oldVal, newVal) returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I did a poor job of converting this loop from it's previous iteration. I updated it to only return when true.

const oldVal = oldCollection[k];
const newVal = newCollection[k];
if (oldVal !== newVal) return isArrayOrObject(oldVal, newVal) ? hasCollectionChanged(oldVal, newVal) : true;
}
}

return false;
};
exports.hasCollectionChanged = hasCollectionChanged;
41 changes: 2 additions & 39 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2607,10 +2607,10 @@ function react(gd, data, layout, config) {
// assume that if there's a config at all, we're reacting to it too,
// and completely replace the previous config
if (config) {
var oldConfig = Lib.extendDeep({}, gd._context);
const oldConfig = Lib.extendDeepAll({}, gd._context);
gd._context = undefined;
setPlotContext(gd, config);
configChanged = diffConfig(oldConfig, gd._context);
configChanged = helpers.hasCollectionChanged(oldConfig, gd._context);
}

if (configChanged) {
Expand Down Expand Up @@ -3028,43 +3028,6 @@ function getDiffFlags(oldContainer, newContainer, outerparts, opts) {
}
}

/*
* simple diff for config - for now, just treat all changes as equivalent
*/
function diffConfig(oldConfig, newConfig) {
var key;

for (key in oldConfig) {
if (key.charAt(0) === '_') continue;
var oldVal = oldConfig[key];
var newVal = newConfig[key];
if (oldVal !== newVal) {
if (Lib.isPlainObject(oldVal) && Lib.isPlainObject(newVal)) {
if (diffConfig(oldVal, newVal)) {
return true;
}
} else if (Array.isArray(oldVal) && Array.isArray(newVal)) {
if (oldVal.length !== newVal.length) {
return true;
}
for (var i = 0; i < oldVal.length; i++) {
if (oldVal[i] !== newVal[i]) {
if (Lib.isPlainObject(oldVal[i]) && Lib.isPlainObject(newVal[i])) {
if (diffConfig(oldVal[i], newVal[i])) {
return true;
}
} else {
return true;
}
}
}
} else {
return true;
}
}
}
}

/**
* Animate to a frame, sequence of frame, frame group, or frame definition
*
Expand Down
41 changes: 41 additions & 0 deletions test/jasmine/tests/plot_api_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3094,6 +3094,47 @@ describe('plot_api helpers', function () {
expect(helpers.hasParent({ 'marker.line': 1 }, attr2)).toBe(true);
});
});

describe('hasCollectionChanged', () => {
it('Returns true if object collection has changed', () => {
expect(
helpers.hasCollectionChanged(
{ captain: 'Leela', deliveryBoy: 'Fry' },
{ captain: 'Leela', deliveryBoy: 'Bender' }
)
).toBe(true);
});

it("Returns false if object collection hasn't changed", () => {
expect(
helpers.hasCollectionChanged(
{ captain: 'Leela', deliveryBoy: 'Fry' },
{ captain: 'Leela', deliveryBoy: 'Fry' }
)
).toBe(false);
});

it('Returns true if array collection has changed', () => {
expect(helpers.hasCollectionChanged(['Zoidberg', 'Hermes'], ['Zoidberg', 'Leela'])).toBe(true);
});

it("Returns false if array collection hasn't changed", () => {
expect(helpers.hasCollectionChanged(['Zoidberg', 'Hermes'], ['Zoidberg', 'Hermes'])).toBe(false);
});

it('Handles nested objects', () => {
expect(
helpers.hasCollectionChanged(
{ level1: { captain: 'Leela', deliveryBoy: 'Fry' } },
{ level1: { captain: 'Leela', deliveryBoy: 'Bender' } }
)
).toBe(true);
});

it('Handles nested arrays', () => {
expect(helpers.hasCollectionChanged([['Zoidberg', 'Hermes']], [['Zoidberg', 'Leela']])).toBe(true);
});
});
});

describe('plot_api edit_types', function () {
Expand Down
Loading