fix: "Map maximum size exceeded" in deepClone #17142
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58779 |
liuxingbaoyu
added
PR: Bug Fix 🐛
| try { | ||
| return deepClone(value, new Map(), true); | ||
| } catch (_) { | ||
| return structuredClone(value); |
There was a problem hiding this comment.
This only works in node v17+, but v8.serialize doesn't work in browsers and other runtimes.
There was a problem hiding this comment.
Can we use structuredClone in Babel 8?
There was a problem hiding this comment.
#16967 (comment)
Unfortunately it is slow, less than half the performance of current implementations.🤦♂️
| function deepClone( | ||
| value: any, | ||
| cache: Map<any, any>, | ||
| allowCircle: boolean, |
There was a problem hiding this comment.
The allowCircle is false but the object actually contains a cycle, this function is now going to cause an infinite loop, right? Can we throw in that case instead? We'd only need to keep track of the current "stack", rather than of all nodes in the map.
| try { | ||
| return deepClone(value, new Map(), true); | ||
| } catch (_) { | ||
| return structuredClone(value); |
There was a problem hiding this comment.
Can we use structuredClone in Babel 8?
| circleSet.clear(); | ||
| throw new Error("Babel-deepClone: Cycles are not allowed in AST"); | ||
| } | ||
| circleSet.add(value); |
There was a problem hiding this comment.
How big is the performance impact of using this set compared to:
- the old behavior with the map
- the behavior with no protection against infinite loops
If it's significantly expensive, we could keep track of the depth we are at in the cloning and only start using the cycle detection logic if it's, for example, >100. It means that we'll be slower in error cases, but faster in the (common) successful case.
In Babel 7, only a workaround was applied, and in Babel 8, a complete fix was made.