Skip to content

{ "moduleResolution": "node" } support incomplete; doesn't understand package.json files #8527

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

Closed
dbaeumer opened this issue May 9, 2016 · 6 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@dbaeumer
Copy link
Member

dbaeumer commented May 9, 2016

From @mattflix on April 29, 2016 21:46

  • VSCode Version: 1.0.0
  • OS Version: Mac OS 10.11.3

Steps to Reproduce:

  1. Create the following project structure.
  2. Open the root folder in VS Code.
  3. Observe errors and poor Intellisense in ./src/entry.js.

It appears that the "Node module resolution" support in VS Code is incomplete, and only understands two variants:

  • how to resolve a single file as a module (e.g., ./src/node_modules/foo.ts),
  • how to resolve a folder as a module that contains an "index" file (e.g., ./src/node_modules/bar/index.ts).

It doesn't understand the third variant:

  • how to resolve a folder as a module that contains a "package" file (e.g., ./src/node_modules/baz/package.json pointing to ./src/node_modules/baz/main.ts).

All three of these variants (in any mixture) are vital to proper support for "Node module resolution" in modular Node project structures.

Also, none of these variants work when the file extensions are .js instead of .ts (even when using a jsconfig.json file or allowJs=true). This is terrible, and makes VS Code useless for navigating many existing Javascript projects. The fact than any of the variants work for .ts files is great, but they should definitely work for .js files!

./tsconfig.json

{
    "compilerOptions": {
        "module": "es6",
        "moduleResolution": "node"
    }
}

./src/entry.ts

import foo from 'foo'; // OK
import bar from 'bar'; // OK
import baz from 'baz'; // [ts] Cannot find module 'baz'.

foo.foo(); // has Intellisense
bar.bar(); // has Intellisense
baz.baz(); // does not have Intellisense

./src/node_modules/foo.ts

export function foo(x: string) {}

./src/node_modules/bar/index.ts

export function bar(y: number) {}

./src/node_modules/baz/package.json

{
    "name": "baz",
    "version": "1.0.0",
    "main": "./main.ts"
}

./src/node_modules/baz/main.ts

export function baz(z: boolean) {}

Copied from original issue: microsoft/vscode#6001

@dbaeumer
Copy link
Member Author

dbaeumer commented May 9, 2016

Moving to TS.

@mhegazy
Copy link
Contributor

mhegazy commented May 9, 2016

The TS module resolution uses "typings" field in package.json to locate modules. the reason is "main": "./main.ts" is not going to work at runtime as .ts will not be executed by node.

see more documentation about module resolution in http://www.typescriptlang.org/docs/handbook/module-resolution.html

@mhegazy mhegazy closed this as completed May 9, 2016
@mhegazy mhegazy added Question An issue which isn't directly actionable in code and removed typescript labels May 9, 2016
@mattflix
Copy link

mattflix commented May 17, 2016

@mhegazy Good point... my use of main.ts in the package.json is somewhat of a typo then. But, my point stands, all aspects of standard Node module resolution, and Intellisense that can be derived thereof, should work for plain .js files, to whatever extent that static analysis of Javascript can support.

My real point was, the { "moduleResolution": "node" } feature doesn't seem to understand that package.json files refer to code via the "main" or "browser" field (which would be nice to be configurable, btw) that should be statically analyzed, and contribute to Intellisense.

@mhegazy
Copy link
Contributor

mhegazy commented May 17, 2016

i think you are asking about #6670, which is handled by #7075

@mattflix
Copy link

Perhaps. Although the discussion in those issues gives me pause as well. The proposed solutions seem to assume that any folder named "node_modules", or code that is located "is up a level", is somehow special. But, this is not the case.

A folder named "node_modules", wherever it appears, simply has prescribed treatment under Node module resolution rules. It is highly desirable to take advantage of these rules for project organization, creating a node_modules folder anywhere in your project hierarchy in order to modularize code. (With zero configuration other than the file structure itself.)

Consider:

/project
    /node_modules
    /app
        /node_modules
        /foo
        /bar

Here, /project/app/node_modules might contain private modules used by the "foo" and "bar" components of the application's source code. Nothing in /project/app/node_modules would be installed or managed by NPM, or considered "read-only". It is just modularized source code. If I want to use Intelliense and refactoring features (esp. renaming) to make changes to code residing anywhere under /project/app, it would be nice (read, very useful) to be able to do so with a minimum of fuss.

Sure, the "top-level" node_modules folder (here /project/node_modules) is very often the place where already-baked NPM modules are installed, but tools should not make this assumption, or limit or fix functionality based on it.

@mhegazy
Copy link
Contributor

mhegazy commented May 17, 2016

i am sorry i do not understand the issue any more. what is the specific ask from the compiler that it does not support today?

@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
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

3 participants