Add `--allowArbitraryExtensions`, a flag for allowing arbitrary extensions on import paths #51435

weswigham · 2022-11-08T00:17:42Z

And, correspondingly, the .d.ext.ts files needed to describe the shape of whatever it is your importing.

Implements #50133

Option name, as always, open to 🚴🏚️.

Thundercraft5 · 2022-11-08T01:04:03Z

A name suggestion: How about --resolveArbitraryExtensions or --allowArbitraryFileExtensions?

jakebailey

This all seems good and straightforward to me; mostly test changes and the actual changes are really nice and minimal.

My only question is about performance; we're doing quick checks to avoid extra FS calls, right?

jakebailey · 2022-11-08T05:59:38Z

src/compiler/parser.ts

@@ -9775,7 +9776,7 @@ namespace IncrementalParser {

 /** @internal */
 export function isDeclarationFileName(fileName: string): boolean {
-    return fileExtensionIsOneOf(fileName, supportedDeclarationExtensions);
+    return fileExtensionIsOneOf(fileName, supportedDeclarationExtensions) || (fileExtensionIs(fileName, Extension.Ts) && stringContains(fileName, ".d."));


I guess we're going so far as to support foo.d.tar.gz.ts? i.e. multiple dots?

Right now we interpret such a name as a declaration file, but will never match it, since we only consider everything after the last dot for the extracted extension. Somewhat on the fence about it right now tbh - I dunno if we need to handle compound extensions or not. Maybe I should adjust the lookup logic so it does?

I honestly have no clue. It seems like someone's going to try to do it... but maybe not?

src/compiler/moduleNameResolver.ts

weswigham · 2022-11-08T18:03:17Z

Yeah, we only do the new fs check when the extension in the import isn't a TS or JS one.

andrewbranch · 2022-11-08T18:16:52Z

I’ve only just glanced at the implementation, but re: naming—in #51171 I introduce allowImportingTsExtensions, and I think I’ve managed to convince myself that its functionality really shouldn’t be combined with this flag you’ve added, because

Importing .ts extensions requires noEmit or emitDeclarationOnly, whereas this flag does not.
If you specifically indicate that you want to import .ts extensions, we can use that knowledge in path completions and auto-imports.

To that end, I would like to brainstorm flag names that don’t seem to imply .ts. I think “arbitrary” is a step in the right direction; maybe “unrecognized”? I haven’t thought of anything I really like yet, just wanted to throw that out and get thoughts.

andrewbranch · 2022-11-08T21:21:37Z

Also: don’t you need corresponding changes in moduleSpecifiers.ts and path completions? E.g., what happens in declaration emit when you have:

// foo.d.html.ts
export declare class CustomHtmlRepresentationThing {}
export declare const someHtml: CustomHtmlRepresentationThing;


// index.ts
import { someHtml } from "./foo.html";
export function getHtml() {
  return someHtml;
}

weswigham · 2022-11-18T01:42:35Z

@andrewbranch Done - I've also simplified things in the module name resolver quite a bit, in part thanks to your refactorings. As an added bonus, .ts file imports now resolve during module resolution, but we still issue retain an error on the import in the checker, for our usual "ts files won't resolve at runtime" reasons. Should make allowImportingTsExtensions pretty easy to implement~

weswigham · 2022-11-18T01:59:08Z

...ference/tscWatch/emitAndErrorUpdates/default/transitive-exports/no-circular-import/export.js

@@ -444,6 +450,8 @@ FsWatches::
  {}

 FsWatchesRecursive::
+/user/username/projects/myproject/lib1:


Just as an FYI, all these extra watchers are here because these tests have an import that looks like "./tools/tools.interface" which now looks for a ./tools/tools.d.interface.ts (for the potential literal tools.interface file) before adding a .js or .ts extension onto the import to find what it currently finds. The additional failed lookup location triggers adding a directory watcher, since now a new file may appear which changes the resolution result~

can you instead change the test to import "./tools/toolsInterface" since the extension part is irrelevant for these tests?

weswigham · 2022-11-18T02:02:13Z

tests/baselines/reference/moduleResolutionNoTsESM.types


 import z from "./z.d.ts";
->z : any
+>z : number


It is probably worth noting that while these types refering to a ts source file in the import do now resolve, the error on the import is still present - it's just added in the checker even on successful resolution now.

andrewbranch · 2022-11-18T17:14:35Z

As an added bonus, .ts file imports now resolve during module resolution, but we still issue retain an error on the import in the checker, for our usual "ts files won't resolve at runtime" reasons. Should make allowImportingTsExtensions pretty easy to implement~

I’ve already done this too 😅 let the merge conflicts continue!

andrewbranch · 2022-11-18T18:05:24Z

src/compiler/checker.ts

@@ -4753,6 +4753,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                        }
                    }
                }
+
+                if (moduleNotFoundError) {
+                    if (tryGetExtensionFromPath(sourceFile.fileName) === tryGetExtensionFromPath(moduleReference)) {


Doesn’t this falsely trigger on import {} from "./foo.ts" when the source file is foo.ts.ts? In my implementation, I couldn’t find a better way than to add a property onto ResolvedModule that indicated whether the .ts extension was actually used to match a .ts file extension.

The error says that the "import path can't end in .ts" not that it can't resolve to TS, so I'd argue it's not a false positive. Honestly, a "./foo.ts" that resolves to "./foo.ts.js" is expempted from the error right now, but I'm questioning more if that's OK - blanket saying "relative import specifiers can't end in .ts unless you pass some flag" just seems easier to understand, and covers weird cjs corners like this, if overzealously.

sheetalkamat · 2022-11-30T23:53:18Z

...ference/tscWatch/emitAndErrorUpdates/default/transitive-exports/no-circular-import/export.js

@@ -444,6 +450,8 @@ FsWatches::
  {}

 FsWatchesRecursive::
+/user/username/projects/myproject/lib1:


can you instead change the test to import "./tools/toolsInterface" since the extension part is irrelevant for these tests?

sheetalkamat · 2022-11-30T23:59:35Z

tests/baselines/reference/nodeModulesExportsBlocksTypesVersions(module=node16).trace.json

@@ -43,6 +43,9 @@
    "File '/node_modules/exports-and-types-versions/package.json' exists according to earlier cached lookups.",
    "Matched 'exports' condition 'types'.",
    "Using 'exports' subpath './yep' with target './types/foo.d.ts'.",
+    "File name '/node_modules/exports-and-types-versions/types/foo.d.ts' has a '.d.ts' extension - stripping it.",
+    "File '/node_modules/exports-and-types-versions/types/foo.ts' does not exist.",


These checks seem non intuitive. When types or exports entry already has "d.ts" extension and we are saying we prefer ".ts" over ".d.ts" even though field said to use ".d.ts" .. Just more configuration error prone i think.

This is generally true all the time - .ts files that correspond to a .d.ts file are higher priority than the .d.ts file, even if your path explicitly uses the .d.ts extension. This is basically to mirror our wildcard include behavior which likewise prefers the .ts file to the .d.ts file.

…s on import paths

weswigham · 2023-01-04T19:22:35Z

@andrewbranch conflicts resolved, there's a minor change in the bundler resolution mode baseline; namely changing module resolution to consistently prefer loading the associated .ts source file, even when the import explicitly says .d.ts. There's also some weirdness with .tsx loading that we really haven't resolved (historically a .js file of the same name would get loaded at a higher priority than the .jsx file, even if you imported the .jsx directly - bunder mode kinda changed that with it's exact match branch, and I've formally encoded the change in behavior for tsx/ts while removing the bundler mode special case in module loading), though maybe it's not terribly important to resolve, since having a a.ts and a.tsx side-by-side is, as far as we're concerned, not really supported anyway.

jakebailey

Not an expert but the code seems fine to me; I think the new name is fine as well. (I'm guessing Andrew will look click a button too but here's my click.)

andrewbranch · 2023-01-09T23:07:58Z

src/testRunner/unittests/tscWatch/emitAndErrorUpdates.ts

        };
        const lib1ToolsInterface: File = {
-            path: `/user/username/projects/myproject/lib1/tools/tools.interface.ts`,
+            path: `/user/username/projects/myproject/lib1/tools/toolsinterface.ts`,


Are filenames like this problematic now?

No, they just add a (higher priority) lookup location which has knock-on effects regarding file watchers in the baseline, so to avoid baseline noise Sheetal asked I simply change the filename.

andrewbranch

Looks good once moduleResolutionSupportsResolvingTsExtensions is deleted.

src/compiler/moduleNameResolver.ts

src/compiler/program.ts

weswigham · 2023-01-09T23:50:52Z

@andrewbranch done~

andrewbranch · 2023-01-23T20:03:32Z

src/compiler/commandLineParser.ts

+    {
+        name: "allowArbitraryExtensions",
+        type: "boolean",
+        affectsModuleResolution: true,


I was gathering module-resolution-affecting compiler options for docs, and noticed this. AFAICT it doesn’t affect module resolution, just whether an error is issued. This should probably be swapped for affectsSemanticDiagnostics.

Is this something that should be in 5.0 beta? I just got to this comment in my email and noticed a PR wasn't sent for this (but I don't know how these two settings matter myself)

weswigham · 2023-01-26T19:48:43Z

It just means we're over-invalidating incremental builds a little bit, since we're tossing resolutions that we could probably reuse if the option value is changed. Honestly not too high impact, so we don't need to squeeze it into the beta if it'll delay things, but we can probably ship a fix in the rc no problem, since a fix is just changing the field name here to another one.

…

On Thu, Jan 26, 2023, 11:40 AM Jake Bailey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/compiler/commandLineParser.ts <#51435 (comment)> : > @@ -1206,6 +1206,14 @@ const commandOptionsWithoutBuild: CommandLineOption[] = [ description: Diagnostics.Enable_importing_json_files, defaultValueDescription: false, }, + { + name: "allowArbitraryExtensions", + type: "boolean", + affectsModuleResolution: true, Is this something that should be in 5.0 beta? I just got to this comment in my email and noticed a PR wasn't sent for this (but I don't know how these two settings matter myself) — Reply to this email directly, view it on GitHub <#51435 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWMAMTG6YIP7AQ4B6R4WSTWULHMLANCNFSM6AAAAAARZXVPPM> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

jakebailey · 2023-01-26T19:49:35Z

I sent #52437 for it just to have it off of my mind 😅

sheetalkamat · 2023-01-26T19:53:33Z

Apart from incremental build, it also means that source files are not reused in LS if affectsModuleResolution is set to true. And that has perf impact so something good to have i guess.

weswigham added the Breaking Change Would introduce errors in existing code label Nov 8, 2022

weswigham requested review from andrewbranch and jakebailey November 8, 2022 00:17

typescript-bot assigned weswigham Nov 8, 2022

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 8, 2022

jakebailey reviewed Nov 8, 2022

View reviewed changes

weswigham requested review from jakebailey and sheetalkamat November 18, 2022 01:39

weswigham commented Nov 18, 2022

View reviewed changes

andrewbranch reviewed Nov 18, 2022

View reviewed changes

sheetalkamat reviewed Dec 1, 2022

View reviewed changes

sandersn added this to Not started in PR Backlog Dec 1, 2022

sandersn moved this from Not started to Waiting on author in PR Backlog Dec 1, 2022

Add --allowNonJsExtensions, a flag for allowing arbitrary extension…

b1fe76f

…s on import paths

weswigham force-pushed the arbitrary-declaration-file-extensions branch from 17e1a6b to b1fe76f Compare January 4, 2023 19:11

Requested test change

e15e077

weswigham requested review from andrewbranch and sheetalkamat January 4, 2023 19:17

weswigham and others added 2 commits January 4, 2023 11:24

Fix lint

2b086a0

Merge branch 'main' into arbitrary-declaration-file-extensions

0a684a0

Rename to allowArbitraryExtensions

8014ef7

weswigham changed the title ~~Add --allowNonJsExtensions, a flag for allowing arbitrary extensions on import paths~~ Add --allowArbitraryExtensions, a flag for allowing arbitrary extensions on import paths Jan 9, 2023

jakebailey approved these changes Jan 9, 2023

View reviewed changes

PR Backlog automation moved this from Waiting on author to Needs merge Jan 9, 2023

Missed two in the rename

226628b

andrewbranch reviewed Jan 9, 2023

View reviewed changes

andrewbranch requested changes Jan 9, 2023

View reviewed changes

src/compiler/moduleNameResolver.ts Show resolved Hide resolved

src/compiler/program.ts Outdated Show resolved Hide resolved

PR Backlog automation moved this from Needs merge to Waiting on author Jan 9, 2023

Remove & inline function, get another rename in

c8069dd

andrewbranch approved these changes Jan 10, 2023

View reviewed changes

PR Backlog automation moved this from Waiting on author to Needs merge Jan 10, 2023

weswigham merged commit 89e928e into microsoft:main Jan 10, 2023

PR Backlog automation moved this from Needs merge to Done Jan 10, 2023

jakebailey mentioned this pull request Jan 10, 2023

Debug failure at resolveExternalModule after allowArbitraryExtensions #52182

Closed

andrewbranch reviewed Jan 23, 2023

View reviewed changes

jakebailey mentioned this pull request Jan 26, 2023

Mark allowArbitraryExtensions as affectsProgramStructure #52437

Merged

sheetalkamat mentioned this pull request Feb 10, 2023

TS Server stats odd seeming file names when resolving json import #52704

Closed

dsherret mentioned this pull request Mar 17, 2023

Any file name with .d. in it ending with .ts is a declaration file in 5.0.2 even without --allowArbitraryExtensions #53319

Closed

fatcerberus mentioned this pull request Aug 25, 2023

allowArbitraryExtensions does not work as expected #55507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--allowArbitraryExtensions`, a flag for allowing arbitrary extensions on import paths #51435

Add `--allowArbitraryExtensions`, a flag for allowing arbitrary extensions on import paths #51435

weswigham commented Nov 8, 2022

Thundercraft5 commented Nov 8, 2022 •

edited

Loading

jakebailey left a comment

jakebailey Nov 8, 2022

weswigham Nov 8, 2022 •

edited

Loading

jakebailey Nov 8, 2022

weswigham commented Nov 8, 2022

andrewbranch commented Nov 8, 2022

andrewbranch commented Nov 8, 2022

weswigham commented Nov 18, 2022

weswigham Nov 18, 2022

sheetalkamat Nov 30, 2022

weswigham Nov 18, 2022

andrewbranch commented Nov 18, 2022

andrewbranch Nov 18, 2022

weswigham Nov 28, 2022

sheetalkamat Nov 30, 2022

sheetalkamat Nov 30, 2022

weswigham Jan 4, 2023

weswigham commented Jan 4, 2023

jakebailey left a comment •

edited

Loading

andrewbranch Jan 9, 2023

weswigham Jan 9, 2023

andrewbranch left a comment

weswigham commented Jan 9, 2023

andrewbranch Jan 23, 2023

jakebailey Jan 26, 2023

weswigham commented Jan 26, 2023 via email

jakebailey commented Jan 26, 2023

sheetalkamat commented Jan 26, 2023

Add --allowArbitraryExtensions, a flag for allowing arbitrary extensions on import paths #51435

Add --allowArbitraryExtensions, a flag for allowing arbitrary extensions on import paths #51435

Conversation

weswigham commented Nov 8, 2022

Thundercraft5 commented Nov 8, 2022 • edited Loading

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Nov 8, 2022

andrewbranch commented Nov 8, 2022

andrewbranch commented Nov 8, 2022

weswigham commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbranch commented Nov 18, 2022

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

weswigham commented Jan 4, 2023

jakebailey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbranch left a comment

Choose a reason for hiding this comment

weswigham commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Jan 26, 2023 via email

jakebailey commented Jan 26, 2023

sheetalkamat commented Jan 26, 2023

Add `--allowArbitraryExtensions`, a flag for allowing arbitrary extensions on import paths #51435

Add `--allowArbitraryExtensions`, a flag for allowing arbitrary extensions on import paths #51435

Thundercraft5 commented Nov 8, 2022 •

edited

Loading

weswigham Nov 8, 2022 •

edited

Loading

jakebailey left a comment •

edited

Loading