Add cloneDeepWithoutLoc #10680
Add cloneDeepWithoutLoc #10680
Conversation
existentialism
added
PR: New Feature 🚀
| export default function cloneNode<T: Object>( | ||
| node: T, | ||
| deep: boolean = true, | ||
| withoutLoc: boolean = false, |
There was a problem hiding this comment.
Deep clones also need to receive the withoutLoc param, which is why the tests are failing.
There was a problem hiding this comment.
Hey @jridgewell, I added withoutLoc to deep clones which means cloned.declarations[0].id.loc needs to be null but I am still receiving {} in the test, any idea why?
JLHwung
added
the
PR: Ready to be Merged
| newNode.loc = null; | ||
| } else { | ||
| newNode.loc = node.loc; | ||
| } | ||
| } | ||
| if (has(node, "leadingComments")) { | ||
| newNode.leadingComments = node.leadingComments; |
There was a problem hiding this comment.
when both deep and withoutLoc is true, we should apply cloneIfNodeOrArray on these comment nodes, same applied to innerComments and trailingComments.
There was a problem hiding this comment.
something like this?
newNode.leadingComments = deep && withoutLoc ?
cloneIfNodeOrArray(node.leadingComments, true, true) : node.leadingComments;
There was a problem hiding this comment.
Yep! And please add a test on the new behaviour.
JLHwung
removed
the
PR: Ready to be Merged
packages/babel-types/test/cloning.js
Outdated
| const cloned = t.cloneNode(node, /* deep */ true, /* withoutLoc */ true); | ||
| expect(cloned.loc).toBeNull(); | ||
| expect(cloned.declarations[0].id.loc).toBeNull(); | ||
| expect(cloned.declarations[0].id.leadingComments).toEqual( |
There was a problem hiding this comment.
The cloned.declarations[0].id.leadingComments will be undefined given that we did not add such comments when constructing node.
Similar to what we did in node.declarations[0].id, we can create the leading comments node
node.leadingComments = [{ loc: {} }]And then we assert that after it is cloned, the loc will be null:
expect(cloned.leadingComments[0].loc).toBeNull()Tip: you can view the node structures on this website, and cloneWithoutLoc aims to eliminate all the loc location information from the AST nodes.
There was a problem hiding this comment.
Thank you for the tip and the link, now I understand what I am doing!
| @@ -11,24 +11,29 @@ function cloneIfNode(obj, deep) { | |||
| obj.type !== "CommentLine" && | |||
| obj.type !== "CommentBlock" | |||
There was a problem hiding this comment.
@JLHwung because of this condition cloneNode will never be called on comments so comment.loc will never be null! I am adding comments to node using:
t.addComment(node, "leading", "leadingComments", true);
node.leadingComments[0].loc = {};
test:
expect(cloned.leadingComments[0].loc).not.toBeNull();
comment type is either CommentLine or CommentBlock! any idea?
Thanks!
There was a problem hiding this comment.
Good catch! I think we should short-circuit this condition when withoutLoc and deep are both true. Otherwise it is inconsistent that after cloneWithoutLoc, the cloned CommentLine still contain loc.
There was a problem hiding this comment.
Yes, we can use this:
((deep && withoutLoc) ||
(obj.type !== "CommentLine" && obj.type !== "CommentBlock"))
But this will throw an error here:
Unknown node type: "CommentBlock"
Unknown node type: "CommentLine"
There was a problem hiding this comment.
@Taym95 Sorry for the long delay! Maybe we could special-case comments and have a small helper function (not exposed as part of the API) cloneCommentWithoutLoc?
github-actions
bot
added
the
outdated
Fixes #10670