[acorn-walk]: baseVisitor[type] is not a function when walking with a plugin

[jakub-g]

Hello,

I tried to walk a JSX file using acorn-walk and acorn-jsx, with the following code:

let acorn = require("acorn")
let walk = require("acorn-walk")
let acornJsx = require("acorn-jsx")

let acornJsxParser = acorn.Parser.extend(acornJsx())
let parsedJsx = acornJsxParser.parse('const foo = () => ( <div></div> )', {sourceType: 'module'})

walk.simple(parsedJsx, {
  ImportDeclaration(node) {
    console.log('ImportDeclaration')
  }
})

and this throws an exception:

C:\git\acorn-walk-test\node_modules\acorn-walk\dist\walk.js:29
    baseVisitor[type](node, st, c);
                     ^

TypeError: baseVisitor[type] is not a function
    at c (C:\git\acorn-walk-test\node_modules\acorn-walk\dist\walk.js:29:22)

The uncaught error is not very friendly, although the code is simple so I figured out this can be fixed by providing the base param, for example like this:

walk.simple(parsedJsx, {
  ImportDeclaration(node) {
    console.log('ImportDeclaration')
  }
}, {
  ...walk.base,
  JSXElement: () => {}
})

I think perhaps this deserves a small code change (checking if baseVisitor[type] exists and logging a friendly message if not), and a small note in the docs.

WDYT? (I'm willing to send a PR)
Jakub

    "acorn": "^6.1.1",
    "acorn-jsx": "^5.0.1",
    "acorn-walk": "^6.1.1"

[marijnh]

Sure, a PR that helps with this would be great.

[jakub-g]

Ok great. So the follow-up question is: what should be the behavior of the walkers (after the PR) around the now-unguarded function calls like baseVisitor[type](...) like this one:

https://github.com/acornjs/acorn/blob/master/acorn-walk/src/index.js#L23

1. (strict) Throw the exception as it is now, but also log a more helpful error message
2. (loose) Just log a warning like Missing visitor for node type JSXElement, ignoring, and continue as if nothing has happened (i.e. as if baseVisitor[type] was an empty function)?
3. (very loose) Continue as if nothing has happened, and do not log any warning -- probably not very good

I think the option 2) makes the most sense but maybe I'm wrong.

The downside of option 1), when parsing code that requires multiple new node type definitions, would be that the user would have to fix them one-by-one, where with 2) they'd see all warnings at once.

[marijnh]

Definitely strict, I think—otherwise the code will just go ahead in a broken form.

[Qix-]

Another alternative (or perhaps, addition) would be to set a Symbol on the returned AST with an array of objects that each introduce extra base node type definitions that are ultimately registered via the plugins themselves.

This would allow e.g. acorn-jsx to add result[acornBaseTypesSymbol].push({ JSXElement: () => ... }) and then the walk methods would automatically do the extension explained in #829 (comment).

Pseudocode:

// in acorn internals
const pluginBaseTypesSymbol = Symbol('acorn plugin base types');

// in acorn-jsx, right before returning AST
if (!ast[pluginBaseTypesSymbol]) {
    ast[pluginBaseTypesSymbol] = []
}
ast[pluginBaseTypesSymbol].push({
    JSXElement: () => {}
});

// in acorn-walk, before performing the walk
const baseVisitor = Object.assign({}, walk.base, ...ast[pluginBaseTypesSymbol], userSuppliedBase);

// in user code
const JSXParser = ...;
const ast = JSXParser.parse('...');
acornWalk.walk(ast, {...}); // OK, no need to specify base types

[thescientist13]

Just wanted to chime in and say I just bumped up into this issue as well, and thankfully found this thread and the linked solution worked for me. 🙇 🙏

walk.simple(parsedJsx, {}, {
  ...walk.base,
  JSXElement: () => {}
})

I think it would be be great to get this added to the docs somewhere, and maybe even added (or linked to / from) the acorn-jsx README as well? I know this is work in progress but if stale, I don't mind trying to submit the relevant PRs. 👍

[glor]

Just had the same error because I forgot to parse code before passing it to a walk.full. A better error message would be a boon.