Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix modeBarButtons issue #6177

Merged
merged 4 commits into from
May 9, 2022

Conversation

nickmelnikov82
Copy link
Member

Fixes dash issue #1157
As @alexcjohnson wrote:

The problem is here where we mutate the input - which is the user-provided array, something we should not alter. Instead we should create a copy with the object inserted in place of the string. Would be a one-line change if we used ramda in plotly.js, but we don't...

@nickmelnikov82 nickmelnikov82 force-pushed the fix-modebarbuttons-issue branch from 98e67de to c1b6ff2 Compare May 5, 2022 10:49
@@ -8,6 +8,7 @@ var isUnifiedHover = require('../fx/helpers').isUnifiedHover;
var createModeBar = require('./modebar');
var modeBarButtons = require('./buttons');
var DRAW_MODES = require('./constants').DRAW_MODES;
var cloneDeep = require('lodash').cloneDeep;
Copy link
Contributor

@archmoj archmoj May 5, 2022

Choose a reason for hiding this comment

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

Thanks very much for the PR.
Please note that lodash is a dev-dependency.
And for various reasons (e.g. size & potential security issues) we are not interested to include that in our bundles.
Alternatively you could use

var extendDeep = require('../../lib').extendDeep;

and then

var customButtons = extendDeep([], context.modeBarButtons);

Or perhaps even better would be to use extendFlat if possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Using the extendFlat function is not possible because the object has a nested structure.

@nickmelnikov82 nickmelnikov82 requested a review from archmoj May 5, 2022 13:38
@archmoj archmoj added the bug something broken label May 5, 2022
@archmoj
Copy link
Contributor

archmoj commented May 5, 2022

Please add a test here:

describe('creates a custom button', function() {

Also I appreciate if you draft 6177_fix.md as described here:
https://github.com/plotly/plotly.js/blob/master/.github/PULL_REQUEST_TEMPLATE.md

Thank you!

@@ -0,0 +1 @@
- Fix modeBarButtons mutate the input, issue [[#1157](https://github.com/plotly/dash/issues/1157)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fix modeBarButtons mutate the input, issue [[#1157](https://github.com/plotly/dash/issues/1157)]
- Fix custom modebar buttons mutate the input [[#6177](https://github.com/plotly/plotly.js/pull/6177)]

@@ -44,7 +45,7 @@ module.exports = function manageModeBar(gd) {
].join(' '));
}

var customButtons = context.modeBarButtons;
var customButtons = extendDeep([], context.modeBarButtons);
Copy link
Contributor

Choose a reason for hiding this comment

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

This deep copy appears to be needed in the first case only.
I suggest you revert the change here and move deep copy inside fillCustomButton function.
For example:

function fillCustomButton(inButtons) {
  var customButtons = extendDeep([], inButtons);
  ...

@archmoj
Copy link
Contributor

archmoj commented May 9, 2022

Thanks very much for the PR.
💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants