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

Allow an import of "foo.js" to be matched by a file "foo.ts" #8895

Merged
8 commits merged into from
Jun 9, 2016
Merged
Show file tree
Hide file tree
Changes from 2 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
9 changes: 7 additions & 2 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -849,13 +849,18 @@ namespace ts {
const extensionsToRemove = [".d.ts", ".ts", ".js", ".tsx", ".jsx"];
export function removeFileExtension(path: string): string {
for (const ext of extensionsToRemove) {
if (fileExtensionIs(path, ext)) {
return path.substr(0, path.length - ext.length);
const extensionless = tryRemoveExtension(path, ext);
if (extensionless !== undefined) {
return extensionless;
}
}
return path;
}

export function tryRemoveExtension(path: string, extension: string): string {
return fileExtensionIs(path, extension) ? path.substring(0, path.length - extension.length) : undefined;
}

export interface ObjectAllocator {
getNodeConstructor(): new (kind: SyntaxKind, pos?: number, end?: number) => Node;
getSourceFileConstructor(): new (kind: SyntaxKind, pos?: number, end?: number) => SourceFile;
Expand Down
28 changes: 23 additions & 5 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ namespace ts {
}

/**
* @param extensions - Either supportedTypeScriptExtensions or if --allowJs, allSupportedExtensions
* @param {boolean} onlyRecordFailures - if true then function won't try to actually load files but instead record all attempts as failures. This flag is necessary
* in cases when we know upfront that all load attempts will fail (because containing folder does not exists) however we still need to record all failed lookup locations.
*/
Expand All @@ -626,13 +627,30 @@ namespace ts {
onlyRecordFailures = !directoryProbablyExists(directory, state.host);
}
}
Copy link
Contributor

@mhegazy mhegazy May 31, 2016

Choose a reason for hiding this comment

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

consider this pattern, split the function into a loadModuleFromFile and loadModuleFromFileWorker. the worker is never called directly except from loaModulefronFile. and loadModuleFromFile becomes:

    function loadModuleFromFile(candidate: string, extensions: string[], failedLookupLocation: string[], onlyRecordFailures: boolean, state: ModuleResolutionState): string {
        return loadModuleFromFileWorker(candidate, extensions, failedLookupLocation, onlyRecordFailures, state) ||
            hasJavaScriptFileExtension(candidate) ? loadModuleFromFileWorker(removeFileExtension(candidate), extensionsm failedLookupLocation, onlyRecordFailures, state) : undefined;
    }

you would want to make it more explicit off course and add some tracing.

Copy link
Author

Choose a reason for hiding this comment

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

Could it be function worker(candidate) in a closure or would that be bad for perf?

Copy link
Contributor

Choose a reason for hiding this comment

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

i do not know :). @vladima thoughts?

return forEach(extensions, tryLoad);

function tryLoad(ext: string): string {
if (ext === ".tsx" && state.skipTsx) {
return undefined;
// First try to keep/add an extension: importing "./foo.ts" can be matched by a file "./foo.ts", and "./foo" by "./foo.d.ts"
const keepOrAddExtension = forEach(extensions, ext => {
if (state.skipTsx && (ext === ".jsx" || ext === ".tsx")) {
Copy link
Member

Choose a reason for hiding this comment

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

If there isn't one, it would be useful to have a function to determine if the file extension is explicitly a JSX extension.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean function isJsxLike(extension: string) { return extension === ".jsx" || extension === ".tsx"; }?

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 need the extra lambda. just revert to the old tryLoad implementation.

return;
}
return tryLoad(fileExtensionIs(candidate, ext) ? candidate : candidate + ext);
});
if (keepOrAddExtension) {
Copy link
Member

Choose a reason for hiding this comment

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

Newline above

return keepOrAddExtension;
}

// Then try stripping a ".js" or ".jsx" extension and replacing it with a TypeScript one, e.g. "./foo.js" can be matched by "./foo.ts" or "./foo.d.ts"
return forEach(supportedJavascriptExtensions, jsExt => {
if (state.skipTsx && jsExt === ".jsx") {
return;
}
const extensionless = tryRemoveExtension(candidate, jsExt);
Copy link
Member

Choose a reason for hiding this comment

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

Newline above

if (extensionless !== undefined) {
return forEach(supportedTypeScriptExtensions, tsExt => tryLoad(extensionless + tsExt));
}
const fileName = fileExtensionIs(candidate, ext) ? candidate : candidate + ext;
});

function tryLoad(fileName: string): string {
if (!onlyRecordFailures && state.host.fileExists(fileName)) {
if (state.traceEnabled) {
trace(state.host, Diagnostics.File_0_exist_use_it_as_a_name_resolution_result, fileName);
Expand Down
14 changes: 7 additions & 7 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

//
// Copyright (c) Microsoft Corporation. All rights reserved.
//
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
Expand Down Expand Up @@ -350,7 +350,7 @@ namespace Utils {
assert.equal(node1.end, node2.end, "node1.end !== node2.end");
assert.equal(node1.kind, node2.kind, "node1.kind !== node2.kind");

// call this on both nodes to ensure all propagated flags have been set (and thus can be
// call this on both nodes to ensure all propagated flags have been set (and thus can be
// compared).
assert.equal(ts.containsParseError(node1), ts.containsParseError(node2));
assert.equal(node1.flags, node2.flags, "node1.flags !== node2.flags");
Expand Down Expand Up @@ -751,7 +751,7 @@ namespace Harness {
(emittedFile: string, emittedLine: number, emittedColumn: number, sourceFile: string, sourceLine: number, sourceColumn: number, sourceName: string): void;
}

// Settings
// Settings
export let userSpecifiedRoot = "";
export let lightMode = false;

Expand Down Expand Up @@ -790,7 +790,7 @@ namespace Harness {
fileName: string,
sourceText: string,
languageVersion: ts.ScriptTarget) {
// We'll only assert invariants outside of light mode.
// We'll only assert invariants outside of light mode.
const shouldAssertInvariants = !Harness.lightMode;

// Only set the parent nodes if we're asserting invariants. We don't need them otherwise.
Expand Down Expand Up @@ -935,7 +935,7 @@ namespace Harness {
libFiles?: string;
}

// Additional options not already in ts.optionDeclarations
// Additional options not already in ts.optionDeclarations
const harnessOptionDeclarations: ts.CommandLineOption[] = [
{ name: "allowNonTsExtensions", type: "boolean" },
{ name: "useCaseSensitiveFileNames", type: "boolean" },
Expand Down Expand Up @@ -1187,7 +1187,7 @@ namespace Harness {
errLines.forEach(e => outputLines.push(e));

// do not count errors from lib.d.ts here, they are computed separately as numLibraryDiagnostics
// if lib.d.ts is explicitly included in input files and there are some errors in it (i.e. because of duplicate identifiers)
// if lib.d.ts is explicitly included in input files and there are some errors in it (i.e. because of duplicate identifiers)
// then they will be added twice thus triggering 'total errors' assertion with condition
// 'totalErrorsReportedInNonLibraryFiles + numLibraryDiagnostics + numTest262HarnessDiagnostics, diagnostics.length

Expand Down Expand Up @@ -1497,7 +1497,7 @@ namespace Harness {
};
testUnitData.push(newTestFile2);

// unit tests always list files explicitly
// unit tests always list files explicitly
const parseConfigHost: ts.ParseConfigHost = {
readDirectory: (name) => []
};
Expand Down
45 changes: 45 additions & 0 deletions tests/baselines/reference/moduleResolution.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//// [tests/cases/compiler/moduleResolution.ts] ////

//// [a.ts]
export default 0;

// No extension: '.ts' added
//// [b.ts]
import a from './a';

// Matching extension
//// [c.ts]
import a from './a.ts';

// '.js' extension: stripped and replaced with '.ts'
//// [d.ts]
import a from './a.js';

//// [jquery.d.ts]
declare var x: number;
export default x;

// No extension: '.d.ts' added
//// [jquery_user_1.ts]
import j from "./jquery";

// '.js' extension: stripped and replaced with '.d.ts'
//// [jquery_user_1.ts]
import j from "./jquery.js"


//// [a.js]
"use strict";
exports.__esModule = true;
exports["default"] = 0;
// No extension: '.ts' added
//// [b.js]
"use strict";
// Matching extension
//// [c.js]
"use strict";
// '.js' extension: stripped and replaced with '.ts'
//// [d.js]
"use strict";
//// [jquery_user_1.js]
"use strict";
36 changes: 36 additions & 0 deletions tests/baselines/reference/moduleResolution.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
=== tests/cases/compiler/a.ts ===
export default 0;
No type information for this code.
No type information for this code.// No extension: '.ts' added
No type information for this code.=== tests/cases/compiler/b.ts ===
import a from './a';
>a : Symbol(a, Decl(b.ts, 0, 6))

// Matching extension
=== tests/cases/compiler/c.ts ===
import a from './a.ts';
>a : Symbol(a, Decl(c.ts, 0, 6))

// '.js' extension: stripped and replaced with '.ts'
=== tests/cases/compiler/d.ts ===
import a from './a.js';
>a : Symbol(a, Decl(d.ts, 0, 6))

=== tests/cases/compiler/jquery.d.ts ===
declare var x: number;
>x : Symbol(x, Decl(jquery.d.ts, 0, 11))

export default x;
>x : Symbol(x, Decl(jquery.d.ts, 0, 11))

// No extension: '.d.ts' added
=== tests/cases/compiler/jquery_user_1.ts ===
import j from "./jquery";
>j : Symbol(j, Decl(jquery_user_1.ts, 0, 6))

// '.js' extension: stripped and replaced with '.d.ts'
=== tests/cases/compiler/jquery_user_1.ts ===
import j from "./jquery.js"
>j : Symbol(j, Decl(jquery_user_1.ts, 0, 6))
>j : Symbol(j, Decl(jquery_user_1.ts, 0, 6))

36 changes: 36 additions & 0 deletions tests/baselines/reference/moduleResolution.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
=== tests/cases/compiler/a.ts ===
export default 0;
No type information for this code.
No type information for this code.// No extension: '.ts' added
No type information for this code.=== tests/cases/compiler/b.ts ===
import a from './a';
>a : number

// Matching extension
=== tests/cases/compiler/c.ts ===
import a from './a.ts';
>a : number

// '.js' extension: stripped and replaced with '.ts'
=== tests/cases/compiler/d.ts ===
import a from './a.js';
>a : number

=== tests/cases/compiler/jquery.d.ts ===
declare var x: number;
>x : number

export default x;
>x : number

// No extension: '.d.ts' added
=== tests/cases/compiler/jquery_user_1.ts ===
import j from "./jquery";
>j : number

// '.js' extension: stripped and replaced with '.d.ts'
=== tests/cases/compiler/jquery_user_1.ts ===
import j from "./jquery.js"
>j : number
>j : number

12 changes: 0 additions & 12 deletions tests/baselines/reference/nameWithFileExtension.errors.txt

This file was deleted.

3 changes: 3 additions & 0 deletions tests/baselines/reference/nameWithFileExtension.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import foo = require('./foo_0.js');
var x = foo.foo + 42;


//// [foo_0.js]
"use strict";
exports.foo = 42;
//// [foo_1.js]
"use strict";
var foo = require('./foo_0.js');
Expand Down
14 changes: 14 additions & 0 deletions tests/baselines/reference/nameWithFileExtension.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
=== tests/cases/conformance/externalModules/foo_1.ts ===
import foo = require('./foo_0.js');
>foo : Symbol(foo, Decl(foo_1.ts, 0, 0))

var x = foo.foo + 42;
>x : Symbol(x, Decl(foo_1.ts, 1, 3))
>foo.foo : Symbol(foo.foo, Decl(foo_0.ts, 0, 10))
>foo : Symbol(foo, Decl(foo_1.ts, 0, 0))
>foo : Symbol(foo.foo, Decl(foo_0.ts, 0, 10))

=== tests/cases/conformance/externalModules/foo_0.ts ===
export var foo = 42;
>foo : Symbol(foo, Decl(foo_0.ts, 0, 10))

17 changes: 17 additions & 0 deletions tests/baselines/reference/nameWithFileExtension.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
=== tests/cases/conformance/externalModules/foo_1.ts ===
import foo = require('./foo_0.js');
>foo : typeof foo

var x = foo.foo + 42;
>x : number
>foo.foo + 42 : number
>foo.foo : number
>foo : typeof foo
>foo : number
>42 : number

=== tests/cases/conformance/externalModules/foo_0.ts ===
export var foo = 42;
>foo : number
>42 : number

Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
decl.ts(1,26): error TS2307: Cannot find module './foo/bar.js'.
decl.ts(1,26): error TS2307: Cannot find module './foo/bar.tx'.
decl.ts(2,26): error TS2307: Cannot find module 'baz'.
decl.ts(3,26): error TS2307: Cannot find module './baz'.


==== decl.ts (3 errors) ====
import modErr = require("./foo/bar.js");
import modErr = require("./foo/bar.tx");
~~~~~~~~~~~~~~
!!! error TS2307: Cannot find module './foo/bar.js'.
!!! error TS2307: Cannot find module './foo/bar.tx'.
import modErr1 = require("baz");
~~~~~
!!! error TS2307: Cannot find module 'baz'.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
decl.ts(1,26): error TS2307: Cannot find module './foo/bar.js'.
decl.ts(1,26): error TS2307: Cannot find module './foo/bar.tx'.
decl.ts(2,26): error TS2307: Cannot find module 'baz'.
decl.ts(3,26): error TS2307: Cannot find module './baz'.


==== decl.ts (3 errors) ====
import modErr = require("./foo/bar.js");
import modErr = require("./foo/bar.tx");
~~~~~~~~~~~~~~
!!! error TS2307: Cannot find module './foo/bar.js'.
!!! error TS2307: Cannot find module './foo/bar.tx'.
import modErr1 = require("baz");
~~~~~
!!! error TS2307: Cannot find module 'baz'.
Expand Down
26 changes: 26 additions & 0 deletions tests/cases/compiler/moduleResolution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// @Filename: a.ts
export default 0;

// No extension: '.ts' added
// @Filename: b.ts
import a from './a';

// Matching extension
// @Filename: c.ts
import a from './a.ts';

// '.js' extension: stripped and replaced with '.ts'
// @Filename: d.ts
import a from './a.js';

// @Filename: jquery.d.ts
declare var x: number;
export default x;

// No extension: '.d.ts' added
// @Filename: jquery_user_1.ts
import j from "./jquery";

// '.js' extension: stripped and replaced with '.d.ts'
// @Filename: jquery_user_1.ts
import j from "./jquery.js"
2 changes: 1 addition & 1 deletion tests/cases/projects/NoModule/decl.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import modErr = require("./foo/bar.js");
import modErr = require("./foo/bar.tx");
import modErr1 = require("baz");
import modErr2 = require("./baz");

Expand Down