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

New replaced node should be traversed #25

Closed
fabiomcosta opened this issue Mar 18, 2014 · 6 comments
Closed

New replaced node should be traversed #25

fabiomcosta opened this issue Mar 18, 2014 · 6 comments
Assignees

Comments

@fabiomcosta
Copy link

This is first of all a question, shouldn't the new nodes added by replace be traversed?

Ex:

types.traverse(ast, function() {
  if ( ... ) {
    this.replace(functionExpressionNode);
  }
});

functionExpressionNode doesn't get traversed.

@benjamn benjamn self-assigned this Mar 18, 2014
@benjamn
Copy link
Owner

benjamn commented Mar 19, 2014

Yeah, this story is pretty broken. I originally didn't want to force replacement nodes to be traversed, but that seems to be what one almost always wants.

You can technically call

types.traverse(
  this.replace(functionExpressionNode)[0],
  arguments.callee
);

but that's admittedly pretty ugly.

@benjamn
Copy link
Owner

benjamn commented Mar 19, 2014

One case where you don't want automatic traversal is when you pass multiple arguments to this.replace and one of the arguments is the current node:

types.traverse(ast, function addLogging(node) {
  if (n.Statement.check(node)) {
    this.replace(
      makeConsoleLogStatementFor(node),
      node // Don't want to traverse this node again!
    );
  }
});

Without some sort of additional check, automatic traversal would result in this traversal never terminating.

@fabiomcosta
Copy link
Author

It seems like having both methods would be the best then.
Add replaceAndTraverse() and keep replace()?

@fabiomcosta
Copy link
Author

Hi @benjamn just to let you know

types.traverse(
  this.replace(functionExpressionNode)[0],
  arguments.callee
);

Worked well for me, thank you! :)

I was doing:

this.replace(functionExpressionNode);
types.traverse(
  functionExpressionNode,
  arguments.callee
);

Which was making me lose all the parent context of functionExpressionNode.

@benjamn
Copy link
Owner

benjamn commented Jul 8, 2014

Fixed by c6fa69d and 92a64f9. You should no longer have to explicitly traverse the new path objects returned from path.replace.

@benjamn benjamn closed this as completed Jul 8, 2014
@fabiomcosta
Copy link
Author

👍

On Tuesday, July 8, 2014, Ben Newman notifications@github.com wrote:

Closed #25 #25.


Reply to this email directly or view it on GitHub
#25 (comment).

Fabio Miranda Costa
twitter: @fabioMiranda
github: fabiomcosta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants