-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Relink less stuff #2375
Changes from all commits
95d540e
1723ed8
3e22d38
5a3d2a6
62e54b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @etpinard There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nicely done! |
||
|
||
w.eval(plotlyjsCode); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really, this is valid ES5? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I believe so, though don't use it in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
, but is it important that these are strings and not numbers? Do they get converted to actual strings during use?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep:
cd[i].id = String(trace.ids[i]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment added in 62e54b7