Skip to content

Don't emit any source files found under node_modules #9421

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.

Sign up for GitHub

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

Merged
billti merged 5 commits into from
Jul 1, 2016
Merged

Don't emit any source files found under node_modules #9421

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5f8cf1a
Don't emit source files found under node_modules
billti Jun 29, 2016
5a45c44
Dont load JavaScript if types packages are present
billti Jun 30, 2016
d8047b6
Renamed API
billti Jun 30, 2016
573bfec
Merge remote-tracking branch 'upstream/master' into dontEmitNodeModules
billti Jun 30, 2016
5e4f13f
Removed one error to avoid full path issues
billti Jun 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Dont load JavaScript if types packages are present
  • Loading branch information
@billti
billti committed Jun 30, 2016
commit 5a45c44eb789f52ceb1aa0e23a230ecb599bfb08
19 changes: 12 additions & 7 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,13 +779,18 @@ namespace ts {
while (true) {
const baseName = getBaseFileName(directory);
if (baseName !== "node_modules") {
const result =
// first: try to load module as-is
loadModuleFromNodeModulesFolder(moduleName, directory, failedLookupLocations, state) ||
// second: try to load module from the scope '@types'
loadModuleFromNodeModulesFolder(combinePaths("@types", moduleName), directory, failedLookupLocations, state);
if (result) {
return result;
// Try to load source from the package
const packageResult = loadModuleFromNodeModulesFolder(moduleName, directory, failedLookupLocations, state);
if (packageResult && hasTypeScriptFileExtension(packageResult)) {
// Always prefer a TypeScript (.ts, .tsx, .d.ts) file shipped with the package
return packageResult;
}
else {
// Else prefer a types package over non-TypeScript results (e.g. JavaScript files)
const typesResult = loadModuleFromNodeModulesFolder(combinePaths("@types", moduleName), directory, failedLookupLocations, state);
if (typesResult || packageResult) {
return typesResult || packageResult;
}
}
}

Expand Down
6 changes: 4 additions & 2 deletions tests/baselines/reference/project/nodeModulesMaxDepthIncreased/amd/maxDepthIncreased/root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
define(["require", "exports", "m1"], function (require, exports, m1) {
define(["require", "exports", "m1", "m4"], function (require, exports, m1, m4) {
"use strict";
m1.f1("test");
m1.f2.a = 10;
m1.f2.person.age = "10"; // Error: Should be number
m1.f2.person.age = "10"; // Should error if loaded the .js files correctly
var r1 = m4.test.charAt(2); // Should error if correctly not using the .js file but using @types info
var r2 = 3 + m4.foo; // Should be OK if correctly using the @types .d.ts file
});
18 changes: 15 additions & 3 deletions ...eference/project/nodeModulesMaxDepthIncreased/amd/nodeModulesMaxDepthIncreased.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
maxDepthIncreased/root.ts(4,1): error TS2322: Type 'string' is not assignable to type 'number'.
maxDepthIncreased/root.ts(7,1): error TS2322: Type 'string' is not assignable to type 'number'.
maxDepthIncreased/root.ts(8,13): error TS2339: Property 'test' does not exist on type 'typeof "C:/src/TypeScript/tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@...'.
Copy link
Copy Markdown
Member Author

@billti billti Jun 30, 2016

Choose a reason for hiding this comment

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

This is causing the issue in the GitHub build... need to figure out where the full path is creeping in to the error message.



==== index.js (0 errors) ====
Expand Down Expand Up @@ -28,11 +29,22 @@ maxDepthIncreased/root.ts(4,1): error TS2322: Type 'string' is not assignable to

exports.f2 = m2;

==== maxDepthIncreased/root.ts (1 errors) ====
==== entry.d.ts (0 errors) ====
export declare var foo: number;

==== maxDepthIncreased/root.ts (2 errors) ====
import * as m1 from "m1";
import * as m4 from "m4";

m1.f1("test");
m1.f2.a = 10;
m1.f2.person.age = "10"; // Error: Should be number

m1.f2.person.age = "10"; // Should error if loaded the .js files correctly
~~~~~~~~~~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'number'.
let r1 = m4.test.charAt(2); // Should error if correctly not using the .js file but using @types info
~~~~
!!! error TS2339: Property 'test' does not exist on type 'typeof "C:/src/TypeScript/tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@...'.
Copy link
Copy Markdown
Member Author

@billti billti Jun 30, 2016

Choose a reason for hiding this comment

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

@RyanCavanaugh I'm seeing this be the actual error (with full path) when a types package has an incorrect type. Is this expected? Doesn't seem very readable.

error

Copy link
Copy Markdown
Member

@RyanCavanaugh RyanCavanaugh Jun 30, 2016

Choose a reason for hiding this comment

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

I'm not sure how this happens. @vladima any ideas?

Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Jun 30, 2016

Choose a reason for hiding this comment

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

file names are always absolute in the VS case. modules do not have a name themselves. so we will use the full file name. this is something we have always done. so not a new thing. we can change that to get a user friendly name for modules to use in errors.

Copy link
Copy Markdown
Member Author

@billti billti Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Right, but even the error message itself it kinda weird and not that helpful does not exist on type 'typeof "<full path to file>". You can't even see the end of the error it's so long. Would at least something like does not exist on module <moduleid> from declaration in <filepath> be possible?

Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Jun 30, 2016

Choose a reason for hiding this comment

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

sure. but again you need a user-friendly version of <module id>, cause now it is the file path.


let r2 = 3 + m4.foo; // Should be OK if correctly using the @types .d.ts file

1 change: 1 addition & 0 deletions ...ines/reference/project/nodeModulesMaxDepthIncreased/amd/nodeModulesMaxDepthIncreased.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"maxDepthIncreased/node_modules/m2/node_modules/m3/index.js",
"maxDepthIncreased/node_modules/m2/entry.js",
"maxDepthIncreased/node_modules/m1/index.js",
"maxDepthIncreased/node_modules/@types/m4/entry.d.ts",
"maxDepthIncreased/root.ts"
],
"emittedFiles": [
Expand Down
5 changes: 4 additions & 1 deletion ...s/baselines/reference/project/nodeModulesMaxDepthIncreased/node/maxDepthIncreased/root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
"use strict";
var m1 = require("m1");
var m4 = require("m4");
m1.f1("test");
m1.f2.a = 10;
m1.f2.person.age = "10"; // Error: Should be number
m1.f2.person.age = "10"; // Should error if loaded the .js files correctly
var r1 = m4.test.charAt(2); // Should error if correctly not using the .js file but using @types info
var r2 = 3 + m4.foo; // Should be OK if correctly using the @types .d.ts file
18 changes: 15 additions & 3 deletions ...ference/project/nodeModulesMaxDepthIncreased/node/nodeModulesMaxDepthIncreased.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
maxDepthIncreased/root.ts(4,1): error TS2322: Type 'string' is not assignable to type 'number'.
maxDepthIncreased/root.ts(7,1): error TS2322: Type 'string' is not assignable to type 'number'.
maxDepthIncreased/root.ts(8,13): error TS2339: Property 'test' does not exist on type 'typeof "C:/src/TypeScript/tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@...'.


==== index.js (0 errors) ====
Expand Down Expand Up @@ -28,11 +29,22 @@ maxDepthIncreased/root.ts(4,1): error TS2322: Type 'string' is not assignable to

exports.f2 = m2;

==== maxDepthIncreased/root.ts (1 errors) ====
==== entry.d.ts (0 errors) ====
export declare var foo: number;

==== maxDepthIncreased/root.ts (2 errors) ====
import * as m1 from "m1";
import * as m4 from "m4";

m1.f1("test");
m1.f2.a = 10;
m1.f2.person.age = "10"; // Error: Should be number

m1.f2.person.age = "10"; // Should error if loaded the .js files correctly
~~~~~~~~~~~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'number'.
let r1 = m4.test.charAt(2); // Should error if correctly not using the .js file but using @types info
~~~~
!!! error TS2339: Property 'test' does not exist on type 'typeof "C:/src/TypeScript/tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@...'.

let r2 = 3 + m4.foo; // Should be OK if correctly using the @types .d.ts file

1 change: 1 addition & 0 deletions ...nes/reference/project/nodeModulesMaxDepthIncreased/node/nodeModulesMaxDepthIncreased.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"maxDepthIncreased/node_modules/m2/node_modules/m3/index.js",
"maxDepthIncreased/node_modules/m2/entry.js",
"maxDepthIncreased/node_modules/m1/index.js",
"maxDepthIncreased/node_modules/@types/m4/entry.d.ts",
"maxDepthIncreased/root.ts"
],
"emittedFiles": [
Expand Down
1 change: 1 addition & 0 deletions tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@types/m4/entry.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/@types/m4/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/m4/entry.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions tests/cases/projects/NodeModulesSearch/maxDepthIncreased/node_modules/m4/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion tests/cases/projects/NodeModulesSearch/maxDepthIncreased/root.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import * as m1 from "m1";
import * as m4 from "m4";

m1.f1("test");
m1.f2.a = 10;
m1.f2.person.age = "10"; // Error: Should be number

m1.f2.person.age = "10"; // Should error if loaded the .js files correctly
let r1 = m4.test.charAt(2); // Should error if correctly not using the .js file but using @types info

let r2 = 3 + m4.foo; // Should be OK if correctly using the @types .d.ts file