Skip to content

ATA: Only download typings for dependencies in top-level package.json #12044

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

Closed
octref opened this issue Nov 4, 2016 · 7 comments
Closed

ATA: Only download typings for dependencies in top-level package.json #12044

octref opened this issue Nov 4, 2016 · 7 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@octref
Copy link

octref commented Nov 4, 2016

When I was verifying ATA in VSCode, I found it downloaded typings for non-top-level deps.

I only added ~20 packages to my package.json, but in my cache I found:

image

IMO only typings for dependencies in package.json should be downloaded. This is also one of the factor that caused npm overload.

@aluanhaddad
Copy link
Contributor

This is arguably correct behavior. See related discussions like #11917

@mhegazy
Copy link
Contributor

mhegazy commented Nov 4, 2016

This is also one of the factor that caused npm overload.

I do not believe this accurate. The npm issue was not caused by valid package installation, but rather by checks for non existing packages. this is based on the information the npm team has shared with us.

We have added a fix for this in TypeScript 2.0.7 release to address the non-existing package issue.

I only added ~20 packages to my package.json, but in my cache I found:

Please note that this is a machine-wide cache. so all projects will contribute to the cache and will use files from the cache if they exist.

@octref
Copy link
Author

octref commented Nov 4, 2016

The npm issue was not caused by valid package installation, but rather by checks for non existing packages.

Say you have a project that has 10 deps, each of them depends on 10 other deps.
For the 100 indirect deps, wouldn't TS need to check which of them have typings?

Though it seems with the fix this issue would be resolved.

@octref
Copy link
Author

octref commented Nov 4, 2016

@mhegazy I'll just follow #11917 and maybe you can close this as a duplicate. Thanks!

@mhegazy
Copy link
Contributor

mhegazy commented Nov 4, 2016

I do not think this is covered by #11917. the automatic type acquisition happens before there are @types packages. it is what acquires these @types packages. so there is a relationship but they are not the same.

Let me ask why you feel this behavior is not correct? do you see typing lags? or it is just concern about disk space?

@octref
Copy link
Author

octref commented Nov 4, 2016

Because it downloads unnecessary typings, that might take very long to finish, depending on internet connection.

For example, for https://github.com/kriasoft/react-starter-kit

react-starter-kit > ls -la node_modules | wc -l
     927

As its package.json has only ~30 packages. Downloading 30 typings should be enough to drive intelliSense.

One concern I have is, although I don't know for sure, my guess is all typings are downloaded with same priority. I'd prefer if typings for deps in package.json can be downloaded first so we can light up intelliSense faster.

On the VSCode side, we are also interested in adding visual indication to the progress of ATA. Instead of showing progress for downloading ~900 typings, showing progress for the ~30 required typings makes more sense.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 24, 2017
@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Needs Investigation This issue needs a team member to investigate its status. labels Sep 18, 2019
@RyanCavanaugh
Copy link
Member

This isn't desirable (typings with unresolved dependencies will have errors and other problems), let alone possible given how npm works (there isn't a way to install a package without its dependencies).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants