-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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 a leak in coverage #5289
Fix a leak in coverage #5289
Changes from all commits
fd40599
dfe1cc7
40a1c86
e803b79
423d579
01128f0
704d1aa
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 |
---|---|---|
|
@@ -9,6 +9,11 @@ | |
|
||
const EMPTY = new Set(); | ||
|
||
export type DeepCyclicCopyOptions = {| | ||
blacklist: Set<string>, | ||
keepPrototype: boolean, | ||
|}; | ||
|
||
// Node 6 does not have gOPDs, so we define a simple polyfill for it. | ||
if (!Object.getOwnPropertyDescriptors) { | ||
// $FlowFixMe: polyfill | ||
|
@@ -26,41 +31,45 @@ if (!Object.getOwnPropertyDescriptors) { | |
|
||
export default function deepCyclicCopy( | ||
value: any, | ||
blacklist: Set<string> = EMPTY, | ||
options?: DeepCyclicCopyOptions = {blacklist: EMPTY, keepPrototype: false}, | ||
cycles: WeakMap<any, any> = new WeakMap(), | ||
): any { | ||
if (typeof value !== 'object' || value === null) { | ||
return value; | ||
} else if (cycles.has(value)) { | ||
return cycles.get(value); | ||
} else if (Array.isArray(value)) { | ||
return deepCyclicCopyArray(value, blacklist, cycles); | ||
return deepCyclicCopyArray(value, options, cycles); | ||
} else { | ||
return deepCyclicCopyObject(value, blacklist, cycles); | ||
return deepCyclicCopyObject(value, options, cycles); | ||
} | ||
} | ||
|
||
function deepCyclicCopyObject( | ||
object: Object, | ||
blacklist: Set<string>, | ||
options: DeepCyclicCopyOptions, | ||
cycles: WeakMap<any, any>, | ||
): Object { | ||
const newObject = Object.create(Object.getPrototypeOf(object)); | ||
const newObject = options.keepPrototype | ||
? Object.create(Object.getPrototypeOf(object)) | ||
: {}; | ||
|
||
// $FlowFixMe: Object.getOwnPropertyDescriptors is polyfilled above. | ||
const descriptors = Object.getOwnPropertyDescriptors(object); | ||
|
||
cycles.set(object, newObject); | ||
|
||
Object.keys(descriptors).forEach(key => { | ||
if (blacklist.has(key)) { | ||
if (options.blacklist && options.blacklist.has(key)) { | ||
delete descriptors[key]; | ||
return; | ||
} | ||
|
||
const descriptor = descriptors[key]; | ||
|
||
if (typeof descriptor.value !== 'undefined') { | ||
descriptor.value = deepCyclicCopy(descriptor.value, EMPTY, cycles); | ||
delete options.blacklist; | ||
descriptor.value = deepCyclicCopy(descriptor.value, options, cycles); | ||
} | ||
|
||
descriptor.configurable = true; | ||
|
@@ -71,16 +80,20 @@ function deepCyclicCopyObject( | |
|
||
function deepCyclicCopyArray( | ||
array: Array<any>, | ||
blacklist: Set<string>, | ||
options: DeepCyclicCopyOptions, | ||
cycles: WeakMap<any, any>, | ||
): Array<any> { | ||
const newArray = []; | ||
const newArray = options.keepPrototype | ||
? // $FlowFixMe: getPrototypeOf an array is OK. | ||
new (Object.getPrototypeOf(array)).constructor(array.length) | ||
: []; | ||
const length = array.length; | ||
|
||
cycles.set(array, newArray); | ||
|
||
for (let i = 0; i < length; i++) { | ||
newArray[i] = deepCyclicCopy(array[i], EMPTY, cycles); | ||
delete options.blacklist; | ||
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. We should be good moving this out of the loop 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. Woah, this is a bug -- it deletes the blacklist on the first key iteration so only the first key can be blacklisted |
||
newArray[i] = deepCyclicCopy(array[i], options, cycles); | ||
} | ||
|
||
return newArray; | ||
|
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.
This test is a duplicate of above
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.
One asserts the default behavior and the other with the flag explicitly passed in
Do you think that's too much?
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 mean its content is literally a copy 😄
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.
Oh wow good catch -- the second test should have: