Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 513340

Summary: Multi-part node require is not being found correctly
Product: [ECD] Orion Reporter: Michael Rennie <Michael_Rennie>
Component: JS ToolsAssignee: Michael Rennie <Michael_Rennie>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P1 CC: curtis.windatt.public, steve_northover
Version: 13.0Flags: curtis.windatt.public: review+
steve_northover: review+
Target Milestone: 14.0   
Hardware: PC   
OS: Mac OS X   
Whiteboard:
Attachments:
Description Flags
Example project showing problem
none
Fix none

Description Michael Rennie CLA 2017-03-08 15:12:42 EST 
Steps:

1. clone the repo: https://github.com/snorthov-org/tone-analyzer-nodejs locally
2. cd to the cloned directory, run 'npm install'
3. open Orion locally (or use the electron app) and open that repo location
4. open the file app.js (in the root of the project)
5. notice that the required entry 'watson-developer-cloud/tone-analyzer/v3' is tagged as being unknown

Looking on-disk the library is present as '~/watson-developer-cloud/tone-analyzer/v3.js' so we could find it. The likely cause is a combination of the fact that there is no 'main' entry in the package.json file and that the dependency is listed as only 'watson-developer-cloud'.

We have logic in our node_modules probing that does not search around node_modules (because the folder is typically huge), instead we look for cues to find dependencies - like "main" and go from there. 

We can improve this a bit to try other lookups, but each lookup we do into node_modules can have a (potentially very) negative impact on performance.
Comment 1 Steve Northover CLA 2017-03-08 15:22:18 EST 
If this is easy/safe etc. we could consider it for Orion 14.  If there are performance or other considerations, likely we leave it.

If I add a main entry in package.json, would this help?
Comment 2 Michael Rennie CLA 2017-03-08 15:24:13 EST 
Created attachment 267178 [details]
Example project showing problem

This is a simplified example that shows the problem. It can be imported into orion.eclipse.org or orionhub.org to reproduce.
Comment 3 Michael Rennie CLA 2017-03-09 10:29:08 EST 
(In reply to Steve Northover from comment #1)
> If this is easy/safe etc. we could consider it for Orion 14.  If there are
> performance or other considerations, likely we leave it.
> 
> If I add a main entry in package.json, would this help?

In this case probably not, simply because the way that the dependency is available on-disk.
Comment 4 Michael Rennie CLA 2017-03-09 12:17:09 EST 
Created attachment 267200 [details]
Fix

The problem boiled down to:

1. not properly returning the file name in the Tern response
2. not trying to look up the correct file based on the module path

The patch fixes those things and removes an unnecessary file extension check and properly returns a failed read back to Tern (when the lib were looking for cannot be found.
Comment 5 Steve Northover CLA 2017-03-09 12:34:10 EST 
Umm ... safe and good ... ?
Comment 6 Michael Rennie CLA 2017-03-09 12:55:15 EST 
(In reply to Steve Northover from comment #5)
> Umm ... safe and good ... ?

Small fix, only targets multi-part requires, and fixes a bug that apparently has always been there.
Comment 7 Curtis Windatt CLA 2017-03-09 13:27:21 EST 
My only concern is that calling _failedRead will now output the 404 to the console when you enter a bogus path.

Otherwise the change makes some obvious improvements (deleting duplicate code, actually returning the file) and makes the code safer by removing the _nodeRead recursion.

I opened a bug to consider handling require on folders in the future (right now we just append .js to the path).
Bug 513402 - Allow node require for folders

I'll hold off on +1 until the 404 is addressed.
Comment 8 Michael Rennie CLA 2017-03-09 13:31:23 EST 
(In reply to Curtis Windatt from comment #7)

> I'll hold off on +1 until the 404 is addressed.

just a simple update on the read call to:

fileclient.read(fileToLoad, false, false, {readIfExists: true})

I'll make the change and push the patch.
Comment 10 Curtis Windatt CLA 2017-03-09 13:39:30 EST 
Much better +1 for 14.0