document this.traverse vs this.visit

[jamestalmage]

update: now that my original question has been answered, This issue is now a request for documentation. Leaving it open because the discussion below is a good starting point for a PR.

Original Post

First a simple explanation of my goal and what I tried

var inputCode = 'if (DEV_ENV) a(); else b();';

assert.equal(transform(inputCode, true), 'a();');
assert.equal(transform(inputCode, false), 'b();');

function transform(code, DEV_ENV) {
  var ast = recast.parse(code);
  ast = types.visit(ast, {
    visitIfStatement: function(path) {
      // traverse 'test' child node *before* evaluating this node
      // other transforms may reduce this to a literal value
      var test = this.visit(path.get('test')); 

      // visitIdentifier never gets called if I use this.traverse
      // var test = this.traverse(path.get('test'));  

      var alternate = path.get('alternate');
      var consequent = path.get('consequent');
      if (n.Literal.check(test)) {
        // naive implementation - actual code handles BlockStatements
        path.replace(this.traverse(test.value ? consequent : alternate));  
      } else {
        this.traverse(consequent);
        this.traverse(alternate);
      }
    },
    visitIdentifier: function(path) {
       if (path.value.name === 'DEV_ENV') {
         path.replace(b.literal());
       }
       return false;
    },
    //...
  });
  return recast.print(ast).code;
}

See the comments about using this.traverse(path.get('test'));. What is the difference between the this.traverse and this.visit? When is it appropriate to use one over the other?

[benjamn]

When a visitor method like visitIfStatement has finished handling a certain path, it somehow needs to call this.visit on all the child paths of that path.

The most convenient/common way to do that is simply to call this.traverse on the same path object, which continues the traversal by visiting the children of that path object. Note that path.node itself is not re-visited, because then no progress would be made.

In hindsight, it might have made more sense to call this.traverse something like this.visitChildren, though at the time I imagined this.traverse might do more than just visit children (maybe it would invoke other pending visitor methods, in an effort to merge multiple AST traversals… but that never really worked out).

In more complicated cases like yours, it's probably better to avoid this.traverse and call this.visit manually on the children that you care about. Both methods set .needToCallTraverse = false, so the visitor won't complain that you forgot to traverse the children.

Just be careful you don't call this.visit on the same path you just visited, since that will lead to infinite recursion.

[jamestalmage]

Just be careful you don't call this.visit on the same path you just visited, since that will lead to infinite recursion.

That is actually what I just did about 15 minutes ago, and it precipitated the "aha moment" where I figured it out myself. Was coming back here to answer my own question, but (as always) you were lightning quick with a great answer. Thanks!

FWIW, I think the names are fine. But I didn't see any documentation on this.visit (much less a comparison of the two), and the answer did not jump right out at me when I looked at the source. I'm updating this issue to a request for documentation. (I may get to a PR myself in a few days).