anacierdem / vscode-requirejs

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

Inline require issue #37

Closed anacierdem closed 6 years ago

anacierdem commented 7 years ago

Cannot navigate over moduleA in the following snippet;

define(function(require) {
    var moduleA = require('moduleB');
});
prantlf commented 7 years ago

If you do not want to re-implement this part of the RequireJS functionality, you could leverage either the requirejs module, or the amodro-trace module for this. They parse such modules and store the direct dependencies in the deps array. They do it also for modules using the array in the define() statement; there is no need to handle these two module kinds differently.

prantlf commented 7 years ago

I looked at possibilities to parse RequireJS modules to get dependencies, AMD or CJS, without writing new parsing code in this extension. Parsing is needed during every symbol lookup; caching the parsed dependencies would improve performance. I need to test how the performance is perceived during real editing.

Test                Time      Dependencies  Parameters  Resolved paths  Inline require  Module access
-----------------------------------------------------------------------------------------------------
testEvalDefine       0.850ms      Yes           No            No               No             No
testAmodroTrace     71.500ms      Yes           No           Yes              Yes            Yes
testAmodroParse     40.500ms      Yes           No            No              Yes             No
testAmodroParseAmd  40.500ms      Yes          Yes            No              Yes             No
testAmodroParseCjs  44.500ms      Yes          Yes            No              Yes             No

Although executing the module file using eval() is the quickest, getting parameter names and supporting inline require would need additional parsing.

Using out-of-the-box functionality of the amodro-trace module supports inline require() statements and returns already resolved module paths, but getting parameter names would need additional parsing, and the full recursive module loading including plugins makes this options not only the slowest, but usable only for complete projects.

Parsing using the internal amodro-trace/lib/parse module does not return already resolved module paths, but that can be done easily using requirejs.toUrl. After a little modification it can return the formal parameter names too. Parsing a CJS module takes a little more time, than an AMD module.

I attached the testing scripts as performance.zip.

prantlf commented 7 years ago

I made an experimental commit to the combined branch of my fork - 6fd8397f93115f572727509dd61e037be325fa18 - to test performance and functionality of a modified amodro-trace/lib/parse for parsing module dependencies. Inline require statements work there. I removed all parsing out of the extension to be able to play better with it.

I would like to get parsing of module dependencies including the formal parameter names to the public interface of amodro-trace.

anacierdem commented 7 years ago

Not tested anything yet, but still seems nice.

Should also look for a way to include amodro-trace as a dependency.

prantlf commented 7 years ago

I went further to do all parsing and lookup using esprima. I tried an extreme - jQuery 2 with 10K lines. Parsing took 100ms and looking up "$" (which is not there) there 30ms. The delay was noticeable on the first lookup, but when repeating "Go to Definition" it could not be perceived any more again. (I built in LRU caches for parsed modules and dependency lists.) But it's not what you will usually do. Parsing and traversing is below 10ms with usual source modules, which you're editing and you want the best performance for the first lookup, where the caches do not help.

I think, that replacing regexes with traversing ES AST would make the extension more reliable. I'm not sure, if leaving the regexes as a super-fast mode would be worth it. I'd wait for some feedback.

I added amodro-trace as a dependency as the last commit to #46. The difference to eval was not perceptible.