Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Js module search depth #6928

Closed
wants to merge 5 commits into from
Closed

Js module search depth #6928

wants to merge 5 commits into from

Conversation

billti
Copy link
Member

@billti billti commented Feb 5, 2016

This is a possible solution for #6670 (load JavaScript files from node_modules).

A concern with searching node_modules and loading JavaScript code is just how much code we might suck in on a project with lots of dependencies (or even a few, if those have a large dependency chain). This adds a setting to specify how many "hops" to go when loading JavaScript files while searching node_modules.

As an example, just using Gulp, with a setting of 1 it loads all the JavaScript files in the 'gulp' package, but does not follow its top level imports. This gives intellisense such as the below.

screen shot 2016-02-05 at 11 03 22 am

Note how the gulp exports are visible, however src is showing as any as this is a re-export of another dependency. With a setting of 2 (or deeper), this resolves also, as shown below:

screen shot 2016-02-05 at 11 04 10 am

A question is what to default this setting at. Even with just Gulp, this loads 147 JavaScript files if loading all dependencies. 0 seems unhelpful. Perhaps 1 is a reasonable default?

@vladima for feedback on the code, and @egamma for feedback on the defaults.

@weswigham
Copy link
Member

Out of curiosity, how feasible would it be to remove the limit but then (likely in the background) generate and cache a flattened definition file for each top level module (so we don't have to keep all that backing JS in the service)? Deps don't usually change all that frequently.

}
if (typeof jsonContent.main === "string") {
const path = normalizePath(combinePaths(candidate, jsonContent.main));
const result = loadNodeModuleFromFile(/*don't add extension*/[""], path, failedLookupLocation, !directoryProbablyExists(getDirectoryPath(path), host), host);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after comment

if(result && result.resolvedModule && result.resolvedModule.resolvedFileName){
if(!fileExtensionIs(result.resolvedModule.resolvedFileName, ".ts")){
// Loaded a non-TypeScript file from a node modules search
result.resolvedModule.isJsFileFromNodeSearch = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not think you really need that. we can just keep the depth for all files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. And after talking to Vlad and reviewing the code, I can likely also just use the flag he already has (isExternalLibrary).

@billti billti force-pushed the jsModuleSearchDepth branch from ace9d3e to cfcf7b5 Compare February 9, 2016 04:19
@billti
Copy link
Member Author

billti commented Feb 9, 2016

Simplified the code by using an existing flag, and corrected a couple of minor nits. Also added the logic to not compile/emit any source code that was found by searching the node_modules hierarchy.

I'll play with this a little more and add some further tests, but would appreciate a glance over to see if I've overlooked some behavior or requirement somewhere.

@billti
Copy link
Member Author

billti commented Feb 13, 2016

Closing this, as it's not going to make 1.8. I have a pull request for master I'll open instead.

@billti billti closed this Feb 13, 2016
@billti billti deleted the jsModuleSearchDepth branch February 13, 2016 21:47
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants