mozilla / testpilot-ga

Mozilla Public License 2.0
3 stars 9 forks source link

Audit files in published npm module #12

Closed pdehaan closed 6 years ago

pdehaan commented 7 years ago

I did a fresh install of testpilot-ga in an empty project, and noticed that my node_modules folder was a huge 184KB. A bit more than I expected, given /package.json:6 has:

"files": "dist",

Everything:

$ tree node_modules
node_modules
└── testpilot-ga
    ├── LICENSE
    ├── README.md
    ├── dist
    │   └── index.js
    ├── docs
    │   ├── CODE_OF_CONDUCT.md
    │   ├── api.md
    │   ├── changelog.md
    │   └── development.md
    ├── example
    │   ├── README.md
    │   ├── addon
    │   │   ├── icons
    │   │   │   └── testpilot-ga-32.png
    │   │   ├── manifest.json
    │   │   └── popup
    │   │       └── example.html
    │   ├── background_scripts
    │   │   └── background.js
    │   ├── package.json
    │   ├── popup
    │   │   └── example.js
    │   └── webpack.config.js
    ├── package.json
    ├── src
    │   └── TestPilotGA.js
    └── yarn.lock

10 directories, 18 files

Total directory size (as fudged by du -sh):

$ du -sh node_modules/testpilot-ga
184K    node_modules/testpilot-ga

Total nested file sizes (similar fudge factor):

$ du -sh node_modules/testpilot-ga/*
 20K    node_modules/testpilot-ga/LICENSE
  4K    node_modules/testpilot-ga/README.md
  8K    node_modules/testpilot-ga/dist/
 20K    node_modules/testpilot-ga/docs/
 40K    node_modules/testpilot-ga/example/
  4K    node_modules/testpilot-ga/package.json
  8K    node_modules/testpilot-ga/src/
 72K    node_modules/testpilot-ga/yarn.lock

Dummy package.json showing dependencies:

$ cat package.json
{
  "name": "testpilot-ga-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "dependencies": {
    "testpilot-ga": "0.3.0"
  }
}
pdehaan commented 7 years ago

Here's probably the more relevant bits:

 ls -lashR node_modules/testpilot-ga | grep -v "drwx"

  8 -rw-r--r--   1 pdehaan  staff   234B Jul 19 04:20 .babelrc
  8 -rw-r--r--   1 pdehaan  staff   883B Jul 18 14:36 .npmignore
 40 -rw-r--r--   1 pdehaan  staff    16K Jul 18 14:35 LICENSE
  8 -rw-r--r--   1 pdehaan  staff   2.2K Jul 20 12:33 README.md
  8 -rw-r--r--   1 pdehaan  staff   2.8K Jul 30 13:27 package.json
144 -rw-r--r--   1 pdehaan  staff    70K Jul 19 04:32 yarn.lock

node_modules/testpilot-ga/dist:
16 -rw-r--r--   1 pdehaan  staff   5.9K Jul 20 12:33 index.js

node_modules/testpilot-ga/docs:
 8 -rw-r--r--   1 pdehaan  staff   2.4K Jul 18 14:31 CODE_OF_CONDUCT.md
16 -rw-r--r--   1 pdehaan  staff   4.4K Jul 20 12:33 api.md
 8 -rw-r--r--   1 pdehaan  staff   245B Jul 20 12:33 changelog.md
 8 -rw-r--r--   1 pdehaan  staff    14B Jul 18 14:34 development.md

node_modules/testpilot-ga/example:
8 -rw-r--r--   1 pdehaan  staff   100B Jul 18 17:59 .eslintrc.json
8 -rw-r--r--   1 pdehaan  staff    86B Jul 18 18:32 .npmignore
8 -rw-r--r--   1 pdehaan  staff   657B Jul 18 18:33 README.md
8 -rw-r--r--   1 pdehaan  staff   452B Jul 20 12:33 package.json
8 -rw-r--r--   1 pdehaan  staff   399B Jul 20 11:03 webpack.config.js

node_modules/testpilot-ga/example/addon:
8 -rw-r--r--   1 pdehaan  staff   532B Jul 18 18:45 manifest.json

node_modules/testpilot-ga/example/addon/icons:
8 -rw-r--r--  1 pdehaan  staff   530B Jul 18 17:59 testpilot-ga-32.png

node_modules/testpilot-ga/example/addon/popup:
8 -rw-r--r--  1 pdehaan  staff   192B Jul 18 18:30 example.html

node_modules/testpilot-ga/example/background_scripts:
8 -rw-r--r--   1 pdehaan  staff   605B Jul 20 12:33 background.js

node_modules/testpilot-ga/example/popup:
8 -rw-r--r--   1 pdehaan  staff   305B Jul 18 18:41 example.js

node_modules/testpilot-ga/src:
16 -rw-r--r--   1 pdehaan  staff   5.3K Jul 20 12:33 TestPilotGA.js

Looks like the biggest offender may be that 70KB yarn.lock file.

pdehaan commented 7 years ago

Ugh, I think files is supposed to be an Array, not a string: https://docs.npmjs.com/files/package.json#files

Running this through my favorite package.json validator at http://package-json-validator.com/ shows the following errors/warnings/recommendations:

{
  "valid": false,
  "errors": [
    "Type for field files, was expected to be array, not string"
  ],
  "warnings": [
    "Missing recommended field: keywords",
    "Missing recommended field: bugs",
    "Missing recommended field: author",
    "Missing recommended field: contributors"
  ],
  "recommendations": [
    "Missing optional field: homepage",
    "Missing optional field: dependencies",
    "Missing optional field: engines"
  ]
}

I don't care about most of the warnings/recommendations. npm can infer the bugs and homepage fields based on the supplied repository. But it may be nice to throw an author in there for grins.

In fact, I'll either do this in a new PR, or be lazy and stack a commit in my outstanding PR so I can avoid a conflict+rebasin'.


UPDATE: Submitted PR #13.