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

[Salsa] load .js files from node packages #6670

Closed
egamma opened this issue Jan 27, 2016 · 6 comments
Closed

[Salsa] load .js files from node packages #6670

egamma opened this issue Jan 27, 2016 · 6 comments
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@egamma
Copy link
Member

egamma commented Jan 27, 2016

From @alexandrudima on January 27, 2016 14:48

Testing #2218

File structure:

  • node_modules
    • something2
      • main.js
      • package.json
  • consumer.js

node_modules/something2/main.js:

var f = function() {
    if (true || true) {
        return 5;
    }
    return '5';
}
exports.x = f();

node_modules/something2/package.json:

{
    "name": "something2",
    "version": "0.0.0",
    "main": "./main.js"
}

consumer.js

var s2 = require('something2');
s2.

Observe: x is not suggested for s2

Copied from original issue: microsoft/vscode#2459

@billti
Copy link
Member

billti commented Jan 27, 2016

We could potentially add this discovery to JavaScript and TypeScript resolution. For TypeScript we would still honor the 'typings' property first if present.

@vladima any thoughts?

@billti
Copy link
Member

billti commented Feb 1, 2016

A concern is this could lead to an explosion of files loaded by following the 'require' chain through the dependencies. It could be possible that only following the chain a certain 'distance' from the program root files would give a usable experience however (e.g. load 'something2' above, but don't try and load anything that it 'require's).

The goal is that our automatic typing acquisition would auto-detect and download any Node modules installed by the user, and that these typings would be used and provide a richer definition than could be inferred from the JavaScript anyway.

On the other hand, this would be relatively simple to add (we already search the node_modules hierarchy and crack the package.json to look for TypeScript definitions), and would add value in situations where .d.ts files aren't present or can't be automatically obtained (e.g. there aren't any for that library, the user is offline, etc.). There are a number of reasons there may not be a typing published (e.g. it may be in the 'long tail' of packages, it could be a private NPM repository inside an enterprise, it could be under development and hasn't been published yet, etc...), but this is likely dwarfed by the usage of the most common modules that typings do exist for.

Our current decision is to get the automatic typing acquisition feature added, and then see if this is still a pain point that needs addressing.

@mhegazy mhegazy closed this as completed Feb 2, 2016
@mhegazy mhegazy added Suggestion An idea for TypeScript Revisit An issue worth coming back to labels Feb 2, 2016
@mhegazy mhegazy modified the milestones: Salsa 0.9, TypeScript 1.8.2 Feb 2, 2016
@massimocode
Copy link

@mhegazy @billti I believe that the automatic typing acquisition is not sufficient for the exact same reasons you state above: "There are a number of reasons there may not be a typing published (e.g. it may be in the 'long tail' of packages, it could be a private NPM repository inside an enterprise, it could be under development and hasn't been published yet, etc...)"

Just to point out my colleague is now using Tern.JS with Sublime text and gets useful intellisense for all of our private NPM repository modules. It would be useful if it was at least configurable so that we could enable it ourselves.

Like you say, the majority of packages have typing files available. We could rely on those where available and perform analysis on the packages for which typing files aren't available.

@mhegazy mhegazy reopened this Feb 2, 2016
@mhegazy mhegazy removed this from the TypeScript 1.8.2 milestone Feb 2, 2016
@mhegazy mhegazy added In Discussion Not yet reached consensus and removed Revisit An issue worth coming back to labels Feb 2, 2016
@mhegazy mhegazy changed the title Salsa does not find vanilla JS node modules [Salsa] load .js files from node packages Feb 2, 2016
@billti
Copy link
Member

billti commented Feb 18, 2016

Pull request #7075 is out for this currently.

@mattflix
Copy link

mattflix commented Apr 25, 2016

Please keep in mind that node_modules folders aren't "only" used for external packages. Rather, they are an intergral part of overall module resolution when using Node, Browserify, Webpack, or any other of these extremely popular tools that implement the Node module resolution algorithm.

So all these feature requests to "better support node" or "packages" aren't necessarily about "getting typings" or providing Intellisense for NPM dependencies only. This is about Javascript project organization and modular programming using the system hierarchical dependencies afforded by the de facto standard Node module resolution algorithm. (And when I say "Javascript", I don't strictly mean Javascript.. but any flavor, simple ES3, ES6, Typescript, etc.)

For example, an extremely useful Javscript project structure is as follows:

  • ${workspaceRoot}/
    • src/
      • node_modules/
        • foo/
          • index.js
        • bar/
          • index.js
      • main.js

Here, note that src/node_modules/ has nothing to do with external packages or NPM. There is no use of NPM in sight. It is simply a place to put application-private modules that can be found by each other and throughout the rest of the application code by name (e.g., require("foo"), import "bar", etc.) with zero configuration and complete avoidance of "../../../../ hell".

Going further, of course, if any external, published modules are required, then another, higher-level node_modules folder can simply be added:

  • ${workspaceRoot}/
    • node_modules/
      • (modules installed via npm)
    • src/
      • node_modules/
        • foo/
          • index.js
        • bar/
          • index.js
      • main.js

And then all of the application code, including the private modules and application code equally, can begin referring to the external dependencies by name too.

This is the beauty of the Node module resolution algorithm. Hierarchical dependencies. Please try to support it as a first-class citizen.

@billti
Copy link
Member

billti commented Jun 28, 2016

#7075 has been merged in now. Closing this.

@billti billti closed this as completed Jun 28, 2016
@mhegazy mhegazy added this to the TypeScript 2.0 milestone Jun 28, 2016
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Jun 28, 2016
@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
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants