Add editor configurable filename-based ATA #40952

jessetrinity · 2020-10-05T20:55:52Z

Filename-based ATA is sometimes undesirable in external and inferred projects. This change allows that behavior to be configured without needing to add a tsconfig.

Adds disableFilenameBasedTypeAcquisition to typeAcquisition, to control whether or not typings are inferred based on .js filenames.
Allows typeAcquisition to be configured by editors for non-configured (no tsconfig) projects.

Fixes #40123

typescript-bot · 2020-10-05T20:55:56Z

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

src/server/editorServices.ts

src/compiler/types.ts

uniqueiniquity · 2020-10-05T21:04:50Z

src/server/editorServices.ts

@@ -3515,7 +3523,7 @@ namespace ts.server {
            const typeAcquisition = proj.typeAcquisition!;
            Debug.assert(!!typeAcquisition, "proj.typeAcquisition should be set by now");
            // If type acquisition has been explicitly disabled, do not exclude anything from the project
-            if (typeAcquisition.enable === false) {
+            if (typeAcquisition.enable === false || typeAcquisition.inferTypings === false) {


Just to clarify - does applySafeList only apply in the "infer from filenames" case?

It removes any project files that appear in the safe list from the project, and adds them to the include list (for which typings are installed unconditionally).

If there is a case where we want to avoid adding files to the include list and remove them from the project, then this would be wrong (but I'm not sure what that is).

uniqueiniquity · 2020-10-05T21:05:45Z

src/server/editorServices.ts

@@ -3632,6 +3640,7 @@ namespace ts.server {
            if (proj.typeAcquisition.enable === undefined) {
                proj.typeAcquisition.enable = hasNoTypeScriptSource(proj.rootFiles.map(f => f.fileName));
            }
+            proj.typeAcquisition.inferTypings = this.hostConfiguration.typeAcquisition?.inferTypings ?? true;


I'm a little surprised by this - it seems like whatever the editor sends on the specific project is unconditionally overwritten by the host config. Is that intended?

That was my intent. Do you think we should sent the setting with openExternalProjects rather than making it configurable through configure?

I guess I don't understand why we would prefer that way.

Yes.. This should be part of open external projects

src/testRunner/unittests/tsserver/typingsInstaller.ts

sheetalkamat · 2020-10-05T22:45:25Z

src/server/editorServices.ts

@@ -3632,6 +3640,7 @@ namespace ts.server {
            if (proj.typeAcquisition.enable === undefined) {
                proj.typeAcquisition.enable = hasNoTypeScriptSource(proj.rootFiles.map(f => f.fileName));
            }
+            proj.typeAcquisition.inferTypings = this.hostConfiguration.typeAcquisition?.inferTypings ?? true;


This seems incorrect.. host.typeAcquisition shouldnt mean for all projects.. external project can configure its on options so it shouldnt use default configured thing at all

I did intend for it to be global. For VS this would be under tools > options. Is there a more preferred way of configuring external projects?

Should openExternalProjects handle the setting typeAcquisition for external projects and then only inferred projects get the setting from the host configuration?

IMO you would at least want to allow an external project to specify what it wants and fall back to the host config.

The setting should be only for inferred projects rather than all.. You should look at compilerOptions for inferred project as they can also differ with projectRoot. You would want similar.

src/server/editorServices.ts

src/server/project.ts

src/server/editorServices.ts

src/jsTyping/jsTyping.ts

src/server/editorServices.ts

sheetalkamat · 2020-10-14T20:53:13Z

src/server/editorServices.ts

@@ -294,6 +304,7 @@ namespace ts.server {
        hostInfo: string;
        extraFileExtensions?: FileExtensionInfo[];
        watchOptions?: WatchOptions;
+        typeAcquisition?: TypeAcquisition;


You shouldn’t need this since the options are per project and there isn’t anything in host

sheetalkamat · 2020-10-14T20:57:43Z

src/compiler/types.ts

@@ -5844,7 +5844,8 @@ namespace ts {
        enable?: boolean;
        include?: string[];
        exclude?: string[];
-        [option: string]: string[] | boolean | undefined;
+        inferTypingsFromFilenames?: boolean;


I think you want apposite of this option so that current type acquisitions in config file behave as if this option is set to false. Ideally you want new behavior to kick in only when the option is set

I think I accounted for that, making so that the behavior is only disabled if explicitly set to false. I was imagining something like Enable/disable filename-based ATA as the text of the setting on the editor side, with it checked by default.

I suppose disableFilenameBasedTypeAcquisition works just as well.

sheetalkamat · 2020-10-15T17:54:53Z

src/server/protocol.ts

@@ -1333,7 +1333,7 @@ namespace ts.server.protocol {
     * For external projects, some of the project settings are sent together with
     * compiler settings.
     */
-    export type ExternalProjectCompilerOptions = CompilerOptions & CompileOnSaveMixin & WatchOptions;
+    export type ExternalProjectCompilerOptions = CompilerOptions & CompileOnSaveMixin & WatchOptions & TypeAcquisition;


ExternalProject has options called typeAcquisition so this type shouldnt be changed.

You can create different type instead you should create a type
export type InferredProjectCompilerOptions = ExternalProjectCompilerOptions & TypeAcquisition and update the usage.

sheetalkamat · 2020-10-15T17:55:57Z

src/server/project.ts

@@ -1827,11 +1831,16 @@ namespace ts.server {
            super.close();
        }

+        setTypeAcquisition(newTypeAcquisition: TypeAcquisition): void {


Wonder if if this.typeAcquisition and set method should be moved to Project since 3 project types use the same method.

sheetalkamat · 2020-10-15T17:57:53Z

src/server/project.ts

                enable: allRootFilesAreJsOrDts(this),
                include: ts.emptyArray,
-                exclude: ts.emptyArray
+                exclude: ts.emptyArray,
+                disableFilenameBasedTypeAcquisition: false


I dont think you need to set this since this is optional

sheetalkamat · 2020-10-16T18:30:24Z

You would want to wait for this merge if this is for 4.2 instead of 4.1 though

jessetrinity · 2020-10-16T18:32:26Z

@sheetalkamat because it is a new feature rather than a bug fix, or because of the protocol changes?

sheetalkamat · 2020-10-16T18:43:34Z

New feature.. i think we are past merging features to 4.1 .. @DanielRosenwasser and @RyanCavanaugh are keeping track of schedule.

minestarks · 2020-10-16T21:05:02Z

I think this is OK for 4.1 but I'll double check with @DanielRosenwasser / @RyanCavanaugh

DanielRosenwasser · 2020-10-16T21:10:01Z

I think that's probably okay, we can always pull it out post-release if we see issues with VS or VS Code Insiders. @mjbvz just to ensure everyone's aware that we're making ATA changes for JS projects.

jessetrinity added 2 commits October 5, 2020 13:44

add typeAcquisition:inferTypings

187576c

remove unused property

52bc8b0

jessetrinity requested a review from sheetalkamat October 5, 2020 20:55

typescript-bot assigned jessetrinity Oct 5, 2020

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Oct 5, 2020

jessetrinity requested review from uniqueiniquity and minestarks October 5, 2020 20:56

uniqueiniquity reviewed Oct 5, 2020

View reviewed changes

sheetalkamat requested changes Oct 5, 2020

View reviewed changes

jessetrinity added 2 commits October 6, 2020 10:45

handle inferred and external projects separately

4113e91

update missed rename

45ae636

sheetalkamat requested changes Oct 6, 2020

View reviewed changes

src/server/editorServices.ts Outdated Show resolved Hide resolved

src/server/editorServices.ts Outdated Show resolved Hide resolved

jessetrinity added 4 commits October 6, 2020 11:33

fix tests

d3a2d41

pass as external compilerOption

67fda70

update test

554a98f

remove hostConfig reference

48eccaa

jessetrinity requested review from sheetalkamat and uniqueiniquity October 13, 2020 18:08

sheetalkamat requested changes Oct 14, 2020

View reviewed changes

jessetrinity added 2 commits October 15, 2020 10:22

change option name

a187137

remove extraneous property

aa46e02

sheetalkamat requested changes Oct 15, 2020

View reviewed changes

add inferredProjectCompilerOptions

b6ee490

jessetrinity requested a review from sheetalkamat October 16, 2020 17:50

sheetalkamat approved these changes Oct 16, 2020

View reviewed changes

jessetrinity merged commit 08e4f36 into microsoft:master Oct 19, 2020

jessetrinity mentioned this pull request Oct 28, 2020

noop in setTypeAcquisiton for undefined #41291

Merged

Add editor configurable filename-based ATA #40952

Add editor configurable filename-based ATA #40952

Uh oh!

Conversation

jessetrinity commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Oct 5, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jessetrinity Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sheetalkamat commented Oct 16, 2020

Uh oh!

jessetrinity commented Oct 16, 2020

Uh oh!

sheetalkamat commented Oct 16, 2020

Uh oh!

minestarks commented Oct 16, 2020

Uh oh!

DanielRosenwasser commented Oct 16, 2020

Uh oh!

Uh oh!

jessetrinity commented Oct 5, 2020 •

edited

Loading

jessetrinity Oct 5, 2020 •

edited

Loading