anacierdem / vscode-requirejs

Provides goto definition functionality for require js modules.
MIT License
21 stars 14 forks source link

Comments in the array of dependencies crash the definition lookup #44

Closed prantlf closed 6 years ago

prantlf commented 6 years ago

A source code like this:

require(['moduleA', // first module 
            'moduleB'], function(a, b) {
    ...
});

Causes the following error:

/usr/share/code/resources/app/out/vs/workbench/workbench.main.js:29
  Unexpected token / in JSON at position 12: SyntaxError: Unexpected token / in JSON at position 12
    at JSON.parse (<anonymous>)
    at ReferenceProvider.getModulesWithPathFromRequireOrDefine (/home/ferdipr/Sources/vscode-requirejs/extension.js:48:16)
    at ReferenceProvider.provideDefinition (/home/ferdipr/Sources/vscode-requirejs/extension.js:309:24)
    ...

The dependency list in square brackets should be handled as JavaScript and not JSON. Possible fixes:

  1. Remove the comments. Quick and easy, but 100% robust comment removal is not so easy.
  2. Eval the module source code with a local define implementation to get the dependencies. Robust and minimum code, because it uses the JavaScript VM itself, but the performance on bigger modules could drop. The requirejs itself does it, but they really need the module callback...
  3. Parse the module source code with a JavaScript parser. Robust, because it uses a JavaScript parser. The performance may not be so bad; it would stop after parsing the function declaration. The amodro-trace wrapper of requirejs does it using esprima to get all dependencies, even the CommonJS style.

I will prepare a PR with the solution 1, because it started to really plague my work now.

I think, that the solution 3 would help maintenance of this extension a lot. Instead of improving the hand-made parsing you could concentrate on other features. You would not need methods like getRequireOrDefineStatements and getModulesWithPathFromRequireOrDefine and probably the whole "testFiles" directory...