Bug 477377

Comment 1 Silenio Quarti 2015-09-14 11:23:38 EDT

1: Create a top level folder (not a project) in "orion.eclipse.org" called "Test";
2: Import the attached zip file (check unzip it);
3: Open "xxxxa.js" and  put the caret over "test1" at line#13;
4: Run Open Declaration command

It fails with "Could not fin declaration" status.

5: open "xxxxb.js", put the caret in "test" line#8;
6: Run open declaration command;  Works fine.

Now:

Repeat 3 and 4;

It works this time.

Comment 2 Silenio Quarti 2015-09-14 11:29:45 EDT

Also, if you comment line#4 and uncomment line#7 in xxxxa.js, it works fine.

Comment 3 Olivier Thomann 2016-05-18 17:14:50 EDT

If you define is like this require("./xxxxb"), it works fine. I'll take this one as I might tweak module loading a bit more. require("xxxxb") expects a module called xxxxb.

Comment 4 Olivier Thomann 2016-05-18 17:26:09 EDT

Without ./, the node module lookup tries to find a module called xxxxb inside the node_module folder etc.
Since this lookup doesn't work, it reports the require value as not loaded.
The current behavior looks ok to me. I would close as WONTFIX.
Silenio, your thoughts?

Comment 5 Michael Rennie 2016-05-19 10:42:40 EDT

(In reply to Olivier Thomann from comment #4)
> Without ./, the node module lookup tries to find a module called xxxxb
> inside the node_module folder etc.
> Since this lookup doesn't work, it reports the require value as not loaded.
> The current behavior looks ok to me. I would close as WONTFIX.
> Silenio, your thoughts?

The problem here is that the require in this case is commonjs-like, so the path in the require is technically an AMD path not a node (node_modules) path.

We should be able to handle this in the commonjs plugin - i.e. resolve and load the correct workspace script (vs. the node_modules one).

Also, the code example should be changed to:

define(function(require) {
    var b = require("xxxxb");
}

to be the proper commonjs define/require use

Comment 6 Olivier Thomann 2016-05-20 13:56:57 EDT

This is actually fixed with the fix for bug 494044. Closing as a dup.

*** This bug has been marked as a duplicate of bug 494044 ***

Comment 7 Michael Rennie 2016-05-24 09:14:04 EDT

The commonjs plugin still needs to be fixed for this bug - the other bug fixed the bad lintinting for commonjs scenarios.

The Tern doc explains that the plugin does not work (sadly):

http://ternjs.net/doc/manual.html#plugin_commonjs

Comment 8 Olivier Thomann 2016-05-24 11:45:59 EDT

(In reply to Michael Rennie from comment #7)
> The commonjs plugin still needs to be fixed for this bug - the other bug
> fixed the bad lintinting for commonjs scenarios.
When I tried Silenio's example, I could open the declaration properly. I'll take a deeper look at this.

Comment 9 Olivier Thomann 2016-06-06 12:53:25 EDT

If I don't provide a .tern-project file, things are working.
If I provide a .tern-project file with:
{
	"plugins": {
		"requirejs": {},
		"commonjs": {}
	},
	"libs": [
		"ecma5"
	],
	"ecmaVersion": 5,
	"loadEagerly": [],
	"sourceType": "script"
}
It doesn't work anymore. If I add:
		"node": {}
it works. This would mean we don't properly load the dependent library.

Comment 10 Olivier Thomann 2016-06-06 12:54:48 EDT

xxxxa.js is defined as:

/*eslint-env browser, commonjs, amd*/

// case 1 - fails
define(function() { var b = require("xxxxb");

// case 2 - works
//define(["xxxxb"], function(b) {
	
	
	function hi () {
		console.log("hi");
	}
	b.test1("str");
	b.test2("str");
	hi();
});

Comment 11 Michael Rennie 2016-06-06 13:05:26 EDT

(In reply to Olivier Thomann from comment #9)
> If I don't provide a .tern-project file, things are working.
> If I provide a .tern-project file with:
> {
> 	"plugins": {
> 		"requirejs": {},
> 		"commonjs": {}
> 	},
> 	"libs": [
> 		"ecma5"
> 	],
> 	"ecmaVersion": 5,
> 	"loadEagerly": [],
> 	"sourceType": "script"
> }
> It doesn't work anymore. If I add:
> 		"node": {}
> it works. This would mean we don't properly load the dependent library.

Thats expected - since the commonjs plugin (by itself or with requirejs, does not have a resolver).

There are a couple of simple-ish things you could try here:

1. change the plugin loading order so that commonjs also gets a resolver. This can be done in the plugins' tern.registerPlugin calls. Currently they are loaded as:

 node -> node_resolve -> commonjs -> modules

You could try switching commonjs and node_resolve, and see if that works.

2. You could have the commonjs plugin be directly dependant on node_resolve without changing the overall plugin loading order.

So add a loadPlugin('node_resolve') call in the commonjs plugins' #registerPlugin call.

Comment 12 Olivier Thomann 2016-06-06 13:10:20 EDT

For 12.0, the quickfix will be modified to add "node" when "commonjs" environment is detected.

Comment 13 Olivier Thomann 2016-06-06 13:23:49 EDT

Delivered. Fix for commonjs resolution will be done during 13.0 dev as part of bug 495552.