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

Relink less stuff #2375

Merged
merged 5 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
19 changes: 12 additions & 7 deletions src/lib/relink_private.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ var isPlainObject = require('./is_plain_object');
* This prevents deepCopying massive structures like a webgl context.
*/
module.exports = function relinkPrivateKeys(toContainer, fromContainer) {
var keys = Object.keys(fromContainer || {});

for(var i = 0; i < keys.length; i++) {
var k = keys[i],
fromVal = fromContainer[k],
toVal = toContainer[k];
for(var k in fromContainer) {
var fromVal = fromContainer[k];
var toVal = toContainer[k];

if(toVal === fromVal) {
continue;
}
if(k.charAt(0) === '_' || typeof fromVal === 'function') {

// if it already exists at this point, it's something
Expand All @@ -37,9 +37,14 @@ module.exports = function relinkPrivateKeys(toContainer, fromContainer) {
}
else if(isArray(fromVal) && isArray(toVal) && isPlainObject(fromVal[0])) {

// filter out data_array items that can contain user objects
// most of the time the toVal === fromVal check will catch these early
// but if the user makes new ones we also don't want to recurse in.
if(k === 'customdata' || k === 'ids') continue;
Copy link
Contributor

@etpinard etpinard Feb 16, 2018

Choose a reason for hiding this comment

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

For reference, we should probably right down somewhere that values in the ids arrays should be strings.

But yeah, 👍 for being safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For reference, we should probably right down somewhere that values in the ids arrays should be strings.

I can put that in its attribute description, but is it important that these are strings and not numbers? Do they get converted to actual strings during use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do they get converted to actual strings during use?

yep: cd[i].id = String(trace.ids[i]);

Copy link
Contributor

Choose a reason for hiding this comment

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

I can put that in its attribute description

That would be great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment added in 62e54b7


// recurse into arrays containers
for(var j = 0; j < fromVal.length; j++) {
if(isPlainObject(fromVal[j]) && isPlainObject(toVal[j])) {
if(isPlainObject(fromVal[j]) && isPlainObject(toVal[j]) && (toVal[j] !== fromVal[j])) {
relinkPrivateKeys(toVal[j], fromVal[j]);
}
}
Expand Down
1 change: 1 addition & 0 deletions tasks/util/make_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ module.exports = function makeSchema(plotlyPath, schemaPath) {
// package is annoying and platform-dependent.
// see https://github.com/tmpvar/jsdom/issues/1782
w.HTMLCanvasElement.prototype.getContext = function() { return null; };
w.URL.createObjectURL = function() { return null; };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@etpinard npm run build works again with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done!


w.eval(plotlyjsCode);

Expand Down
103 changes: 103 additions & 0 deletions test/jasmine/tests/lib_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1984,6 +1984,109 @@ describe('Test lib.js:', function() {
});
});
});

describe('relinkPrivateKeys', function() {
it('ignores customdata and ids', function() {
var fromContainer = {
customdata: [{_x: 1, _y: 2, a: 3}],
ids: [{_i: 4, j: 5}]
};
var toContainer = {
customdata: [{a: 6}],
ids: [{j: 7}]
};

Lib.relinkPrivateKeys(toContainer, fromContainer);

expect(toContainer.customdata[0]._x).toBeUndefined();
expect(toContainer.customdata[0]._y).toBeUndefined();
expect(toContainer.ids[0]._i).toBeUndefined();
});

it('ignores any values that are ===', function() {
var accesses = 0;

var obj = {
get _x() { accesses++; return 1; },
Copy link
Contributor

Choose a reason for hiding this comment

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

Really, this is valid ES5?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I believe so, though don't use it in src as it's not supported by IE or Safari. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh actually I was reading the browser compatibility table wrong... in fact looks like this basic setter/getter usage is fine even in IE and Safari, though I still feel like we should avoid it as it can make things confusing. I've only ever used it for testing and debugging. If, for example, a property gets set but you can't figure out where it's getting set, attach a setter for that property that throws an error.

set _x(v) { accesses++; }
};
var array = [obj];
var array2 = [obj];

var fromContainer = {
x: array,
y: array,
o: obj
};
var toContainer = {
x: array,
y: array2,
o: obj
};

Lib.relinkPrivateKeys(toContainer, fromContainer);

expect(accesses).toBe(0);

obj._x = 2;
expect(obj._x).toBe(1);
expect(accesses).toBe(2);
});

it('reinserts other private keys if they\'re not already there', function() {
var obj1 = {a: 10, _a: 11};
var obj2 = {a: 12, _a: 13};
function f1() { return 1; }
function f2() { return 2; }

var fromContainer = {
a: 1,
_a: 2,
_b: 3,
_c: obj1,
_d: obj1,
f: f1, // functions are private even without _
g: f1,
array: [{a: 3, _a: 4, _b: 5, f: f1, g: f1}],
o: {a: 6, _a: 7, _b: 8},
array2: [{a: 9, _a: 10}],
o2: {a: 11, _a: 12}
};
fromContainer._circular = fromContainer;
fromContainer._circular2 = fromContainer;
var toContainer = {
a: 21,
_a: 22,
_c: obj2,
f: f2,
array: [{a: 23, _a: 24, f: f2}],
o: {a: 26, _a: 27},
x: [28],
_x: 29
};
toContainer._circular = toContainer;

Lib.relinkPrivateKeys(toContainer, fromContainer);

var expected = {
a: 21,
_a: 22,
_b: 3,
_c: obj2,
_circular: toContainer,
_circular2: fromContainer,
_d: obj1,
f: f2,
g: f1,
array: [{a: 23, _a: 24, _b: 5, f: f2, g: f1}],
o: {a: 26, _a: 27, _b: 8},
x: [28],
_x: 29
};

expect(toContainer).toEqual(expected);
});
});
});

describe('Queue', function() {
Expand Down