Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: Update config diff check method to handle nested arrays#7579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
camdecoster merged 13 commits intomasterfromcam/7572/update-config-diff-check
Oct 15, 2025

Conversation

@camdecoster
Copy link
Contributor

@camdecostercamdecoster commentedOct 10, 2025•
edited
Loading

Description

Update config diff check to handle nested arrays.

Closes#7572.

Changes

  • Linting/formatting
  • Update config diff check method to handle nested arrays
  • Move method to helpers file
  • Add tests for method

Testing

  • Be on master
  • Run this mock in plotly.js DevTools:
    {"data": [    {"error_x": {"array": [100000.0,100000.0,100000.0],"type":"data","visible":true    },"type":"scatter","x": [1554370371547.085,1554370471547.085,1554370571547.085],"y": [6,10,2]    }  ],"layout": {"xaxis": {"type":"date"    }  },"config": {"modeBarButtons": [[]]  }}
  • Be aware that entering the next command will cause an infinite recursive loop, so add a debugger statement inreact before you do to avoid crashing your browser/computer
  • Callreact from the browser DevTools console with the following command:
    Plotly.react(gd, gd.data, gd.layout, { modeBarButtons: [ [] ] })
  • Click the debugger continue button a number of times and note that it just keeps happening
  • Switch to this branch
  • Run through the same process again
  • Click the debugger continue button and note that it only runs through once

Notes

  • The current diff method doesn't handle nested arrays properly, so the diff check will always return true in this case
  • It didn't cause a problem before because the old code ran throughreact once and that was it. The new code can make a call toreact recursively, which is triggered by the above example.
  • It only happens with themodeBarButtons config option because that's the only one that takes a nested array as a value

TODO

  • Add draftlog

@camdecostercamdecoster changed the titleCam/7572/update-config-diff-checkfix: Update config diff check method to handle nested arraysOct 10, 2025
@camdecostercamdecoster marked this pull request as ready for reviewOctober 10, 2025 22:50
returnfalse;
};

constAX_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
ContributorAuthor

Choose a reason for hiding this comment

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

Can do.

*@param {Object|Array} oldCollection: Old version of collection to compare
*@param {Object|Array} newCollection: New version of collection to compare
*/
consthasCollectionChanged=(oldCollection,newCollection)=>{
Copy link
Contributor

@emilyklemilyklOct 14, 2025•
edited
Loading

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 likeareObjectsEqual() since the function itself doesn't care whetheroldCollection andnewCollection are versions of the same object or not; that's a matter of how the function is used.

Copy link
ContributorAuthor

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. WouldareCollectionsEqual 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 maybecollectionsAreEqual?) 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
ContributorAuthor

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.

if(Object.keys(oldCollection).length!==Object.keys(newCollection).length)returntrue;

for(constkinoldCollection){
if(k.startsWith('_'))returnfalse;
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 beif (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 returntrue from within the loop. Line 535 will returnfalse sometimes and short-circuit the whole loop ifhasCollectionChanged(oldVal, newVal) returnsfalse.

Copy link
ContributorAuthor

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 whentrue.

@emilykl
Copy link
Contributor

@camdecoster Infinite loop is resolved, but I am not able to update the modebar buttons successfully by callingPlotly.react().

This is the line I am running in the browser console:

Plotly.react(gd,gd.data,gd.layout,{modeBarButtons:[['zoom2d','pan2d'],['autoScale2d','resetScale2d']]});

I would expect to see new buttons show up in the modebar but I see nothing. I am also seeing thathasCollectionChanged() seems to returnfalse in that case but I haven't dug deeper than that.

@emilykl
Copy link
Contributor

Also have you been able to learn anything about the intentions behindthis test? Unclear whether it was deliberately forbidden for a philosophical reason, or just because it was complicated to implement.

@camdecoster
Copy link
ContributorAuthor

Also have you been able to learn anything about the intentions behindthis test? Unclear whether it was deliberately forbidden for a philosophical reason, or just because it was complicated to implement.

I've only looked at it to understand why it was failing. In this case, it was because of the bad loop conversion. I'll see if I can understand it a bit better and report back.

@emilykl
Copy link
Contributor

I've only looked at it to understand why it was failing. In this case, it was because of the bad loop conversion. I'll see if I can understand it a bit better and report back.

I ask because the wording of the test case ("should not try to transition when theconfig has changed") seems to directly oppose the functionality you've added (making sure a transition happens duringreact() when the config has changed). I don't see any reason why that's a problem, but the person who added the test originally apparently thought that shouldn't happen.

@camdecoster
Copy link
ContributorAuthor

I ask because the wording of the test case ("should not try to transition when theconfig has changed") seems to directly oppose the functionality you've added (making sure a transition happens duringreact() when the config has changed). I don't see any reason why that's a problem, but the person who added the test originally apparently thought that shouldn't happen.

Okay, so config changes shouldn't be animated perthis comment. The second call toreact that I added handles the situation where both the config and some attribute that should be animated were changed. On the second pass through, the config isn't changing, so that should trigger the transition (if one should occur).

So, the test is there to make sure no transitions happen when a config change occurs. This might actually add a new feature that didn't exist before the changes toreact in#7475: now you can pass in changes to the config and other transition-triggering changes and they should both get handled.

emilykl reacted with thumbs up emoji

@camdecoster
Copy link
ContributorAuthor

Infinite loop is resolved, but I am not able to update the modebar buttons successfully by callingPlotly.react().

This was a consequence of the loop logic error. I tried it after the fix and it worked. Could you try again with the latest changes?

@emilykl
Copy link
Contributor

@camdecoster Thanks for the clear explanation, that all makes sense.

I've tested again and it seems to be working as expected.

Copy link
Contributor

@emilyklemilykl left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe add a few more Jasmine tests forcollectionsAreEqual() if you have the bandwidth, but not required.

@camdecostercamdecoster merged commitbe3cd47 intomasterOct 15, 2025
6 of 7 checks passed
@camdecostercamdecoster deleted the cam/7572/update-config-diff-check branchOctober 15, 2025 22:53
});

it('Returns false for null values',()=>{
expect(helpers.collectionsAreEqual(null,null)).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make sense for this case to returntrue?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I suppose, but that's not how the function is supposed to be used. It would require an additional branch to the conditional check and I didn't feel like adding that. If you feel strongly about it, I'll add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah I don't feel strongly about it. My only suggestion would be to note these details in the function docstring.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The JSDoc param specs show the args should be an array or object. Do you think more would be necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah my bad I skimmed over the jsdoc, if that conventionally meansnull is not a valid input I'm satisfied 👍

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@emilyklemilyklemilykl approved these changes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG]: Plotly.react with config that includes modeBarButtons broken in v3.1.1

4 participants

@camdecoster@emilykl@alexshoe
[8]ページ先頭
©2009-2025 Movatter.jp