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: Memory leak when deep cloning in babel-core #14583

Merged
merged 7 commits into from Jun 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/babel-core/package.json
Expand Up @@ -41,11 +41,9 @@
"./lib/config/files/index.js": "./lib/config/files/index-browser.js",
"./lib/config/resolve-targets.js": "./lib/config/resolve-targets-browser.js",
"./lib/transform-file.js": "./lib/transform-file-browser.js",
"./lib/transformation/util/clone-deep.js": "./lib/transformation/util/clone-deep-browser.js",
"./src/config/files/index.ts": "./src/config/files/index-browser.ts",
"./src/config/resolve-targets.ts": "./src/config/resolve-targets-browser.ts",
"./src/transform-file.ts": "./src/transform-file-browser.ts",
"./src/transformation/util/clone-deep.ts": "./src/transformation/util/clone-deep-browser.ts"
"./src/transform-file.ts": "./src/transform-file-browser.ts"
},
"dependencies": {
"@ampproject/remapping": "^2.1.0",
Expand Down
19 changes: 0 additions & 19 deletions packages/babel-core/src/transformation/util/clone-deep-browser.ts

This file was deleted.

37 changes: 30 additions & 7 deletions packages/babel-core/src/transformation/util/clone-deep.ts
@@ -1,9 +1,32 @@
import v8 from "v8";
import cloneDeep from "./clone-deep-browser";

export default function (value) {
if (v8.deserialize && v8.serialize) {
return v8.deserialize(v8.serialize(value));
//https://github.com/babel/babel/pull/14583#discussion_r882828856
function deepClone(value: any, cache: Map<any, any>): any {
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
function deepClone(value: any, cache: Map<any, any>): any {
function deepClone<T extends object>(value: T, cache: Map<T, T>): T

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that T must be the union of all the types of the nested objects of value (since the cache Map potentially contains all of them), so "this function returns T" isn't much useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is an internal implementation that is not exported.

if (value !== null) {
if (cache.has(value)) return cache.get(value);
let cloned: any;
if (Array.isArray(value)) {
cloned = new Array(value.length);
for (let i = 0; i < value.length; i++) {
cloned[i] =
typeof value[i] !== "object" ? value[i] : deepClone(value[i], cache);
}
} else {
cloned = {};
const keys = Object.keys(value);
for (let i = 0; i < keys.length; i++) {
const key = keys[i];
cloned[key] =
typeof value[key] !== "object"
? value[key]
: deepClone(value[key], cache);
}
}
cache.set(value, cloned);
return cloned;
}
return cloneDeep(value);
return value;
}

export default function <T>(value: T): T {
if (typeof value !== "object") return value;
return deepClone(value, new Map());
}
40 changes: 39 additions & 1 deletion packages/babel-core/test/api.js
Expand Up @@ -261,7 +261,45 @@ describe("api", function () {
},
);

it("transformFromAstSync should not mutate the AST", function () {
it("transformFromAst should generate same code with different cloneInputAst", function () {
const program = `//test1
/*test2*/var/*test3*/ a = 1/*test4*/;//test5
//test6
var b;
`;
const node = parseSync(program);
const { code } = transformFromAstSync(node, program, {
plugins: [
function () {
return {
visitor: {
Identifier: function (path) {
path.node.name = "replaced";
},
},
};
},
],
});
const { code: code2 } = transformFromAstSync(node, program, {
cloneInputAst: false,
plugins: [
function () {
return {
visitor: {
Identifier: function (path) {
path.node.name = "replaced";
},
},
};
},
],
});

expect(code).toBe(code2);
});

it("transformFromAst should not mutate the AST", function () {
const program = "const identifier = 1";
const node = parseSync(program);
const { code } = transformFromAstSync(node, program, {
Expand Down