Don't allow `.ts` to appear in an import #9646

ghost · 2016-07-12T16:47:21Z

I'd like for us to issue an error specific to this issue, because people will be confused if we just output that we can't find ./foo.ts and they see that it's right there.
Unfortunately I'm not sure how to hook up loadModuleFromFile to the error diagnostics system (as opposed to resolution tracing), can anyone help with this?

vladima · 2016-07-12T17:42:08Z

I would not put error reporting into module resolution but instead report error in program if resolved module ended up to be .ts file

ghost · 2016-07-13T19:16:00Z

@vladima See c6d91f9

DanielRosenwasser · 2016-08-01T22:35:53Z

src/compiler/diagnosticMessages.json

@@ -1943,6 +1943,10 @@
        "category": "Error",
        "code": 2689
    },
+    "Module name should not include a '.ts' extension: '{0}'.": {


Try making this

An import path should not include a '.ts' extension. Consider importing '{0}' instead.

An import path should not end with a '{0}' extension. Consider importing '{1}' instead.

Since this error is being used for .tsx as well.

DanielRosenwasser · 2016-08-02T19:50:47Z

src/compiler/utilities.ts

-        return forEach(supportedTypeScriptExtensionsNonDts, extension => fileExtensionIs(fileName, extension));
+    /** Return ".ts" or ".tsx" if that is the extension. */
+    export function tryExtractTypeScriptExtensionNonDts(fileName: string): string | undefined {
+        return find(supportedTypeScriptExtensionsNonDts, extension => fileExtensionIs(fileName, extension));


Don't you want to give the same error if you use .d.ts as well?

Also, can you add a test that imports from a file ending in .d.ts?

If people add a .d.ts extension they may be doing unnecessary work, but it won't cause runtime errors because declaration references don't translate to imports in the output. In contrast, a .ts extension will either fail to import (because the output file has a .js extension) or ask the JS engine to load typescript code.

If people add a .d.ts extension they may be doing unnecessary work, but it won't cause runtime errors because declaration references don't translate to imports in the output.

.d.ts imports don't just get elided. If you use any imported entities as values, then we still emit the import itself. Try it out:

foo.d.ts

export declare var foo;

bar.ts

import * as v from "./foo.d.ts"; v.foo;

The current emit for bar.js is

"use strict"; var v = require("./foo.d.ts"); v.foo;

ghost · 2016-08-03T14:26:23Z

The latest commit would mean we no longer allow a package.json "typings" to specify "foo" when the typings are in "foo.d.ts". We have no tests that relied on that behavior, and it doesn't seem to be a style we've recommended, but still, it's a breaking change.

According to the spec at #2338, it looks like it requires that the package.json "typings" not have an extension — LOAD_AS_DIRECTORY simply calls LOAD_AS_FILE, which always adds an extension. The spec has gotten out-of-date, though, as it doesn't include anything about stripping JS extensions (added by #8895).

sandersn · 2016-08-15T16:07:22Z

src/compiler/core.ts

@@ -109,6 +109,17 @@ namespace ts {
        return undefined;
    }

+    /** Works like Array.prototype.find, returning `undefined` if no element satisfying the predicate is found. */
+    export function tryFind<T>(array: T[], predicate: (element: T, index: number) => boolean): T | undefined {


Hmm, I expected find to have the type that tryFind has right now. Why does find take callback and not predicate?
I forgot to mention it when I ran into it last week while trying to use find.

If we rename tryFind to find, what should we call find? mustFind?

findMap? Let me go see if this function is in hoogle...

It's the equivalent of bind (>>=) or something like Data.Foldable.concatMap except that it takes Array<T> to Exception<T>. So, yeah, findMap is not bad, or mapFirst or mapSingle.

sandersn · 2016-08-17T14:16:21Z

👍

DanielRosenwasser · 2016-08-18T20:53:47Z

src/compiler/utilities.ts

@@ -2713,6 +2713,13 @@ namespace ts {
        return forEach(supportedTypeScriptExtensions, extension => fileExtensionIs(fileName, extension));
    }

+    /** Return ".ts" or ".tsx" if that is the extension. */


…criptExtensions and rename

rbuckton · 2016-08-22T17:06:20Z

The latest commit would mean we no longer allow a package.json "typings" to specify "foo" when the typings are in "foo.d.ts". We have no tests that relied on that behavior, and it doesn't seem to be a style we've recommended, but still, it's a breaking change.

@andy-ms This change in 359c8b1 breaks vscode extensions as vscode uses an extensionless path for "typings" in the vscode-languageserver, vscode-languageserver-types, vscode-languageclient, and vscode-jsonrpc packages. This could be a problem for anyone depending on specific versions of these packages.

rbuckton · 2016-08-22T17:11:06Z

I filed microsoft/vscode-languageserver-node#76 to track this issue for vscode.

msftclas added the cla-not-required label Jul 12, 2016

ghost force-pushed the no_ts_extension branch from 6df731d to f59b3a5 Compare July 12, 2016 17:01

ghost force-pushed the no_ts_extension branch from adc7aa2 to c6d91f9 Compare July 12, 2016 18:21

ghost assigned vladima Jul 14, 2016

Andy Hanson added 2 commits July 21, 2016 13:42

Don't allow .ts to appear in an import

448a480

Add specific error message for unwanted '.ts' extension

a8c05a9

ghost force-pushed the no_ts_extension branch from c6d91f9 to a8c05a9 Compare July 21, 2016 20:51

DanielRosenwasser reviewed Aug 1, 2016
View reviewed changes

Andy Hanson added 2 commits August 2, 2016 06:18

Merge branch 'master' into no_ts_extension

2821d98

Improve error message

0f134ed

DanielRosenwasser reviewed Aug 2, 2016
View reviewed changes

Don't allow ".d.ts" extension in an import either.

359c8b1

Merge branch 'master' into no_ts_extension

3de8c22

ghost assigned sandersn Aug 15, 2016

sandersn reviewed Aug 15, 2016
View reviewed changes

Rename 'find' functions

2eb159e

DanielRosenwasser reviewed Aug 18, 2016
View reviewed changes

Andy Hanson added 3 commits August 18, 2016 14:11

Move supportedTypescriptExtensionsWithDtsFirst next to supportedTypeS…

8fc17af

…criptExtensions and rename

Merge branch 'master' into no_ts_extension

297cb50

Fix tests

b452469

ghost merged commit d2d5d42 into master Aug 19, 2016

ghost deleted the no_ts_extension branch August 19, 2016 13:49

rbuckton mentioned this pull request Aug 22, 2016

Extensionless "typings" entries in package.json no longer work as of TypeScript 2.1.0-dev.20160820 microsoft/vscode-languageserver-node#76

Closed

ghost mentioned this pull request Aug 22, 2016

Allow "typings" in a package.json to be missing its extension #10480

Merged

mhegazy mentioned this pull request Aug 29, 2016

Cannot find module X... #10567

Closed

This was referenced Sep 3, 2016

Loader not working with typescript 2.0.2 (RC) s-panferov/awesome-typescript-loader#221

Closed

Remove file extensions while resolving dependencies. s-panferov/awesome-typescript-loader#222

Merged

realityfilter mentioned this pull request Sep 3, 2016

build error with typescript 2.0.2 RC rbuckton/reflect-metadata#43

Closed

vladima mentioned this pull request Nov 8, 2016

Path mappings have unintuitive behavior if user accidentally adds an extension #12098

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

typescript-bot unassigned vladima Jun 19, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't allow `.ts` to appear in an import #9646

Don't allow `.ts` to appear in an import #9646

ghost commented Jul 12, 2016

vladima commented Jul 12, 2016

ghost commented Jul 13, 2016

DanielRosenwasser Aug 1, 2016

weswigham Aug 1, 2016

DanielRosenwasser Aug 2, 2016

DanielRosenwasser Aug 2, 2016

ghost Aug 2, 2016

DanielRosenwasser Aug 2, 2016 •

edited

Loading

ghost commented Aug 3, 2016

sandersn Aug 15, 2016

ghost Aug 15, 2016

sandersn Aug 15, 2016

sandersn Aug 15, 2016

sandersn commented Aug 17, 2016

DanielRosenwasser Aug 18, 2016

rbuckton commented Aug 22, 2016 •

edited

Loading

rbuckton commented Aug 22, 2016

Don't allow .ts to appear in an import #9646

Don't allow .ts to appear in an import #9646

Conversation

ghost commented Jul 12, 2016

vladima commented Jul 12, 2016

ghost commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

ghost commented Aug 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Aug 17, 2016

Choose a reason for hiding this comment

rbuckton commented Aug 22, 2016 • edited Loading

rbuckton commented Aug 22, 2016

Don't allow `.ts` to appear in an import #9646

Don't allow `.ts` to appear in an import #9646

DanielRosenwasser Aug 2, 2016 •

edited

Loading

rbuckton commented Aug 22, 2016 •

edited

Loading