Skip to content

NPM 7 causes ATA to discover too many dependencies #44130

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
uniqueiniquity opened this issue May 17, 2021 · 4 comments Β· Fixed by #47164
Closed

NPM 7 causes ATA to discover too many dependencies #44130

uniqueiniquity opened this issue May 17, 2021 · 4 comments Β· Fixed by #47164
Assignees
Labels
Bug A bug in TypeScript Domain: JavaScript The issue relates to JavaScript specifically Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@uniqueiniquity
Copy link
Contributor

Bug Report

πŸ”Ž Search Terms

"npm 7"

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

N/A

πŸ’» Code

//@filename: index.js
'use strict';
var debug = require('debug');
var express = require('express');
var path = require('path');
var favicon = require('serve-favicon');
var logger = require('morgan');
var cookieParser = require('cookie-parser');
var bodyParser = require('body-parser');

package.json

{
  "name": "express-app11",
  "version": "0.0.0",
  "private": true,
  "scripts": {
    "start": "node app"
  },
  "description": "ExpressApp11",
  "author": {
    "name": ""
  },
  "dependencies": {
    "body-parser": "^1.15.0",
    "cookie-parser": "^1.4.0",
    "debug": "^2.2.0",
    "express": "^4.14.0",
    "morgan": "^1.7.0",
    "pug": "^2.0.0-beta6",
    "serve-favicon": "^2.3.0"
  },
  "engines": {
    "node": "~6.10.x"
  }
}

πŸ™ Actual behavior

Typings installer log:

Found package names: ["accepts","acorn","acorn-globals","align-text","array-flatten","asap","babel-runtime","babel-types","babylon","basic-auth","body-parser","bytes","call-bind","camelcase","center-align","character-parser","clean-css","cliui","content-disposition","content-type","cookie","cookie-parser","cookie-signature","core.js","core-js","debug","decamelize","depd","destroy","doctypes","ee-first","encodeurl","escape-html","esutils","etag","express","finalhandler","forwarded","fresh","function-bind","get-intrinsic","has","has-symbols","http-errors","inherits","is-buffer","is-core-module","is-expression","is-promise","is-regex","js-stringify","jstransformer","kind-of","lazy-cache","lodash","longest","media-typer","merge-descriptors","methods","mime","mime-db","mime-types","morgan","ms","negotiator","object-assign","on-finished","on-headers","parseurl","path-parse","path-to-regexp","promise","proxy-addr","pug","pug-attrs","pug-code-gen","pug-error","pug-filters","pug-lexer","pug-linker","pug-load","pug-parser","pug-runtime","pug-strip-comments","pug-walk","qs","range-parser","raw-body","regenerator-runtime","repeat-string","resolve","right-align","safer-buffer","send","serve-favicon","serve-static","statuses","to-fast-properties","toidentifier","token-stream","type-is","uglify-js","uglify-to-browserify","unpipe","utils-merge","vary","void-elements","window-size","with","wordwrap","yargs"]

πŸ™‚ Expected behavior

Typings installer log:

Found package names: ["body-parser","cookie-parser","core.js","debug","express","morgan","pug","serve-favicon"]

Investigation notes

ATA discovers typings from the node modules folder by parsing each depedency's package.json and using the _requiredBy field to determine whether it's a top-level dependency or not.

In NPM 7, this field was removed, so every dependency is considered top-level and has its typings installed.

@uniqueiniquity
Copy link
Contributor Author

#7179 (comment) asks the same question that I have, but was never answered πŸ€·β€β™‚οΈ

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: JavaScript The issue relates to JavaScript specifically labels May 18, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.4.0 milestone May 18, 2021
@andrewbranch andrewbranch added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 30, 2021
@minestarks minestarks assigned zkat and unassigned jessetrinity Nov 23, 2021
@minestarks
Copy link
Member

@zkat I'm not sure if there's a better way for Typing Installer to find actual top-level dependencies. What's your take?

@zkat
Copy link
Contributor

zkat commented Nov 23, 2021

This information should be encoded in package-lock.json now. But also, if you want to make it package manager agnostic, is there a reason why you can't filter by "what's in package.json's dependencies"?

Also I don't know what ATA is haha

@minestarks
Copy link
Member

@zkat Welp, this is a good excuse to dive in :). ATA = Automatic Type Acquisition. I'm not sure about the rationale behind the current behavior, but I'm sure it can be improved as Typing Installer has been around for a while.

zkat added a commit to zkat/TypeScript that referenced this issue Dec 16, 2021
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

Fixes: microsoft#44130
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Dec 16, 2021
zkat added a commit to zkat/TypeScript that referenced this issue Dec 16, 2021
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

Fixes: microsoft#44130
zkat added a commit to zkat/TypeScript that referenced this issue Dec 16, 2021
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

Fixes: microsoft#44130
zkat added a commit to zkat/TypeScript that referenced this issue Jan 5, 2022
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

Fixes: microsoft#44130
zkat added a commit to zkat/TypeScript that referenced this issue Jan 5, 2022
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

This also adds support for ATA acquiring types for scoped packages.

Fixes: microsoft#44130
zkat added a commit to zkat/TypeScript that referenced this issue Jan 5, 2022
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

This also adds support for ATA acquiring types for scoped packages.

Fixes: microsoft#44130
zkat added a commit to zkat/TypeScript that referenced this issue Jan 7, 2022
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

This also adds support for ATA acquiring types for scoped packages.

Fixes: microsoft#44130
zkat added a commit that referenced this issue Jan 7, 2022
ATA tried to use the `_requiredBy` field to determine toplevel deps,
but this is not portable. Not only is it unavailable in npm@>=7, but
neither Yarn nor pnpm write this metadata to node_modules pkgjsons.

This also adds support for ATA acquiring types for scoped packages.

Fixes: #44130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: JavaScript The issue relates to JavaScript specifically Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants