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

treat ambient non-aliased 'require' as commonjs 'require' #11819

Merged
merged 1 commit into from
Oct 24, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 29 additions & 4 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12819,16 +12819,41 @@ namespace ts {
}

// In JavaScript files, calls to any identifier 'require' are treated as external module imports
if (isInJavaScriptFile(node) &&
isRequireCall(node, /*checkArgumentIsStringLiteral*/true) &&
// Make sure require is not a local function
!resolveName(node.expression, (<Identifier>node.expression).text, SymbolFlags.Value, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined)) {
if (isInJavaScriptFile(node) && isCommonJsRequire(node)) {
return resolveExternalModuleTypeByLiteral(<StringLiteral>node.arguments[0]);
}

return getReturnTypeOfSignature(signature);
}

function isCommonJsRequire(node: Node) {
Copy link
Member

Choose a reason for hiding this comment

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

May be we need to use this function in completions.ts to determine whether to give module names as completion list?

Copy link
Member

Choose a reason for hiding this comment

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

May be we need to use this function in completions.ts to determine whether to give module names as completion list?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

May be we need to use this function in completions.ts to determine whether to give module names as completion list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

certainly but it is not related to the original issue so I'd put it into different PR

if (!isRequireCall(node, /*checkArgumentIsStringLiteral*/true)) {
return false;
}
// Make sure require is not a local function
const resolvedRequire = resolveName(node.expression, (<Identifier>node.expression).text, SymbolFlags.Value, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined);
if (!resolvedRequire) {
// project does not contain symbol named 'require' - assume commonjs require
return true;
}
// project includes symbol named 'require' - make sure that it it ambient and local non-alias
if (resolvedRequire.flags & SymbolFlags.Alias) {
return false;
}

const targetDeclarationKind = resolvedRequire.flags & SymbolFlags.Function
? SyntaxKind.FunctionDeclaration
: resolvedRequire.flags & SymbolFlags.Variable
? SyntaxKind.VariableDeclaration
: SyntaxKind.Unknown;
if (targetDeclarationKind !== SyntaxKind.Unknown) {
const decl = getDeclarationOfKind(resolvedRequire, targetDeclarationKind);
// function/variable declaration should be ambient
return isInAmbientContext(decl);
}
return false;
}

function checkTaggedTemplateExpression(node: TaggedTemplateExpression): Type {
return getReturnTypeOfSignature(getResolvedSignature(node));
}
Expand Down
21 changes: 21 additions & 0 deletions tests/baselines/reference/ambientRequireFunction.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//// [tests/cases/compiler/ambientRequireFunction.ts] ////

//// [node.d.ts]


declare function require(moduleName: string): any;

declare module "fs" {
export function readFileSync(s: string): string;
}

//// [app.js]
/// <reference path="node.d.ts"/>

const fs = require("fs");
const text = fs.readFileSync("/a/b/c");

//// [app.js]
/// <reference path="node.d.ts"/>
var fs = require("fs");
var text = fs.readFileSync("/a/b/c");
27 changes: 27 additions & 0 deletions tests/baselines/reference/ambientRequireFunction.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
=== tests/cases/compiler/app.js ===
/// <reference path="node.d.ts"/>

const fs = require("fs");
>fs : Symbol(fs, Decl(app.js, 2, 5))
>require : Symbol(require, Decl(node.d.ts, 0, 0))
>"fs" : Symbol("fs", Decl(node.d.ts, 2, 50))

const text = fs.readFileSync("/a/b/c");
>text : Symbol(text, Decl(app.js, 3, 5))
>fs.readFileSync : Symbol(readFileSync, Decl(node.d.ts, 4, 21))
>fs : Symbol(fs, Decl(app.js, 2, 5))
>readFileSync : Symbol(readFileSync, Decl(node.d.ts, 4, 21))

=== tests/cases/compiler/node.d.ts ===


declare function require(moduleName: string): any;
>require : Symbol(require, Decl(node.d.ts, 0, 0))
>moduleName : Symbol(moduleName, Decl(node.d.ts, 2, 25))

declare module "fs" {
export function readFileSync(s: string): string;
>readFileSync : Symbol(readFileSync, Decl(node.d.ts, 4, 21))
>s : Symbol(s, Decl(node.d.ts, 5, 33))
}

30 changes: 30 additions & 0 deletions tests/baselines/reference/ambientRequireFunction.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
=== tests/cases/compiler/app.js ===
/// <reference path="node.d.ts"/>

const fs = require("fs");
>fs : typeof "fs"
>require("fs") : typeof "fs"
>require : (moduleName: string) => any
>"fs" : "fs"

const text = fs.readFileSync("/a/b/c");
>text : string
>fs.readFileSync("/a/b/c") : string
>fs.readFileSync : (s: string) => string
>fs : typeof "fs"
>readFileSync : (s: string) => string
>"/a/b/c" : "/a/b/c"

=== tests/cases/compiler/node.d.ts ===


declare function require(moduleName: string): any;
>require : (moduleName: string) => any
>moduleName : string

declare module "fs" {
export function readFileSync(s: string): string;
>readFileSync : (s: string) => string
>s : string
}

15 changes: 15 additions & 0 deletions tests/baselines/reference/localRequireFunction.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//// [app.js]

function require(a) {
return a;
}

const fs = require("fs");
const text = fs.readFileSync("/a/b/c");

//// [app.js]
function require(a) {
return a;
}
var fs = require("fs");
var text = fs.readFileSync("/a/b/c");
18 changes: 18 additions & 0 deletions tests/baselines/reference/localRequireFunction.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== tests/cases/compiler/app.js ===

function require(a) {
>require : Symbol(require, Decl(app.js, 0, 0))
>a : Symbol(a, Decl(app.js, 1, 17))

return a;
>a : Symbol(a, Decl(app.js, 1, 17))
}

const fs = require("fs");
>fs : Symbol(fs, Decl(app.js, 5, 5))
>require : Symbol(require, Decl(app.js, 0, 0))

const text = fs.readFileSync("/a/b/c");
>text : Symbol(text, Decl(app.js, 6, 5))
>fs : Symbol(fs, Decl(app.js, 5, 5))

24 changes: 24 additions & 0 deletions tests/baselines/reference/localRequireFunction.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
=== tests/cases/compiler/app.js ===

function require(a) {
>require : (a: any) => any
>a : any

return a;
>a : any
}

const fs = require("fs");
>fs : any
>require("fs") : any
>require : (a: any) => any
>"fs" : "fs"

const text = fs.readFileSync("/a/b/c");
>text : any
>fs.readFileSync("/a/b/c") : any
>fs.readFileSync : any
>fs : any
>readFileSync : any
>"/a/b/c" : "/a/b/c"

17 changes: 17 additions & 0 deletions tests/cases/compiler/ambientRequireFunction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// @module: commonjs
// @allowJs: true
// @outDir: ./out/

// @filename: node.d.ts

declare function require(moduleName: string): any;

declare module "fs" {
export function readFileSync(s: string): string;
}

// @filename: app.js
/// <reference path="node.d.ts"/>

const fs = require("fs");
const text = fs.readFileSync("/a/b/c");
11 changes: 11 additions & 0 deletions tests/cases/compiler/localRequireFunction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// @module: commonjs
// @allowJs: true
// @outDir: ./out/

// @filename: app.js
function require(a) {
return a;
}

const fs = require("fs");
const text = fs.readFileSync("/a/b/c");
10 changes: 5 additions & 5 deletions tests/webTestServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ function dir(dirPath: string, spec?: string, options?: any) {
// fs.rmdirSync won't delete directories with files in it
function deleteFolderRecursive(dirPath: string) {
if (fs.existsSync(dirPath)) {
fs.readdirSync(dirPath).forEach((file, index) => {
fs.readdirSync(dirPath).forEach((file) => {
const curPath = path.join(path, file);
if (fs.statSync(curPath).isDirectory()) { // recurse
deleteFolderRecursive(curPath);
Expand All @@ -141,7 +141,7 @@ function deleteFolderRecursive(dirPath: string) {
}
};

function writeFile(path: string, data: any, opts: { recursive: boolean }) {
function writeFile(path: string, data: any) {
ensureDirectoriesExist(getDirectoryPath(path));
fs.writeFileSync(path, data);
}
Expand Down Expand Up @@ -208,7 +208,7 @@ enum RequestType {
Unknown
}

function getRequestOperation(req: http.ServerRequest, filename: string) {
function getRequestOperation(req: http.ServerRequest) {
if (req.method === "GET" && req.url.indexOf("?") === -1) {
if (req.url.indexOf(".") !== -1) return RequestType.GetFile;
else return RequestType.GetDir;
Expand Down Expand Up @@ -258,7 +258,7 @@ function handleRequestOperation(req: http.ServerRequest, res: http.ServerRespons
break;
case RequestType.WriteFile:
processPost(req, res, (data) => {
writeFile(reqPath, data, { recursive: true });
writeFile(reqPath, data);
});
send(ResponseCode.Success, res, undefined);
break;
Expand Down Expand Up @@ -306,7 +306,7 @@ http.createServer((req: http.ServerRequest, res: http.ServerResponse) => {
log(`${req.method} ${req.url}`);
const uri = url.parse(req.url).pathname;
const reqPath = path.join(process.cwd(), uri);
const operation = getRequestOperation(req, reqPath);
const operation = getRequestOperation(req);
handleRequestOperation(req, res, operation, reqPath);
}).listen(port);

Expand Down