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

Automatic Type Acquisition based on filename matching pulls in too many irrelevant typings #40123

Closed
minestarks opened this issue Aug 18, 2020 · 5 comments · Fixed by #40952
Closed
Assignees
Labels
Domain: JavaScript The issue relates to JavaScript specifically Fix Available A PR has been opened for this issue

Comments

@minestarks
Copy link
Member

Consider a simple JavaScript project with files like the following

/flight.js
/platform.js
/myfile.js

The names flight and platform seem innocuous enough, but their mere presence causes Automatic Type Acquisition to pull in @types/flight and @types/platform. Because these words are so common, I would argue it's highly unlikely that the user actually needs the typings for these libraries - much more likely that the files are just regular files in the user's project. I think ATA's behavior is a little too eager, and causes unnecessary files and symbols to be added to the project context. There's also no visual indication that this is actually happening (but you can confirm it by checking TS Server logs).

This behavior happens in VS Code if you have the files open in the editor. But it occurs in VS if you have them present in your project at all.

I propose one or more of the following solutions:

  • Disable this filename-matching behavior entirely
  • Trim back the allow-list considerably to only include obvious libraries like jquery.
  • Give a visual indicator to the user that ATA is doing this, and give them a way to disable it

@DanielRosenwasser

@DanielRosenwasser DanielRosenwasser added the Domain: JavaScript The issue relates to JavaScript specifically label Aug 19, 2020
@sheetalkamat
Copy link
Member

This has heuristic that is at:

if ((!resolvedModule || !resolutionExtensionIsTSOrJson(resolvedModule.extension)) &&

where it does ATA if the resolved module is not ts or json and is referenced using non relative name. This is been around for some time but i think the reason was that eg if you resolved to node_modules\mocha you instead want .d.ts file for it instead. May be adding check for isExternalLibraryImport might be a good addition to check there.

@minestarks
Copy link
Member Author

It's been like this forever yes, I think the original intent (@RyanCavanaugh would know) was to detect third party libraries in non-module based code, e.g. a project that just has jquery.js or jquery.min.js copied in. There are lots of existing VS projects that do this. How do we detect that that file is a dependency, and not user code? At the time we assumed filename matching with an allow-list might be a good heuristic. I'm not so sure of that anymore.

@sheetalkamat
Copy link
Member

isExternalLibraryImport property of the resolution would let us know that since that indicates the resolution is from node_modules

@minestarks
Copy link
Member Author

minestarks commented Aug 19, 2020

Thanks for the pointer. To be clear, in the scenario I describe, there is no node_modules, and we still intend to do ATA.

@jessetrinity
Copy link
Contributor

I think a good solution is a combination of bullet points 2 + 3.

Give a visual indicator to the user that ATA is doing this, and give them a way to disable it

VSCode already shows a status bar for this message and I intend to add one to VS:
image

Contributors for other editor will likely find this issue and if they are also trying to address ATA notifications.

Trim back the allow-list considerably to only include obvious libraries like jquery.

We have data from npm and VS that tells us which packages are downloaded most frequently. We can shorten the list to the top 10-20 from those lists. These include the expected popular packages like lodash, jquery, and bootstrap.

With both changes, users would be getting typings only for popular packages that are probably the jqueryies they are looking for, and if not, be aware of what they are getting through the notification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: JavaScript The issue relates to JavaScript specifically Fix Available A PR has been opened for this issue
Projects
None yet
5 participants