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

cost-of-modules reports too small sizes #40

Open
nknapp opened this issue May 7, 2017 · 9 comments
Open

cost-of-modules reports too small sizes #40

nknapp opened this issue May 7, 2017 · 9 comments

Comments

@nknapp
Copy link

nknapp commented May 7, 2017

I don't know, how important that is to you, but I think that the sizes that are reported are sometimes smaller than the actual sizes. For example

> mkdir test
> cd test/
> npm init -y 
Wrote to /home/nknappmeier/tmp/test/package.json:
[...]

> npm install --save thought
npm WARN prefer global thought@1.1.2 should be installed with -g
test@1.0.0 /home/nknappmeier/tmp/test
└─┬ thought@1.1.2 
  ├── archy@1.0.0 
[...]

> cost-of-modules

Making sure dependencies are installed
npm install --production

npm WARN test@1.0.0 No description
npm WARN test@1.0.0 No repository field.

Calculating...


┌───────────┬──────────────┬───────┐
│ name      │ children     │ size  │
├───────────┼──────────────┼───────┤
│ thought   │ 118          │ 8.63M │
├───────────┼──────────────┼───────┤
│ 1 modules │ 113 children │ 8.43M │
└───────────┴──────────────┴───────┘

> du -hs .
13M	.

While cost-of-modules only reports 8.43M of this project (that has only a package.json and the thought-dependency), while du -hs reports 13M. Maybe it does not take into account, that even a very small file uses 4kb of space, depending on the filesystems block-size.

@nknapp
Copy link
Author

nknapp commented May 7, 2017

I was motivated an wrote my own analzyer analyze-module-size which reports the sizes in a slightly different way (output the whole tree with aggregated block-based sizes for each dependency).

@siddharthkp
Copy link
Owner

Correct size is definitely the most important thing.

Can you share your package.json and I'll look into what's happening

@siddharthkp
Copy link
Owner

Great work on analyze module size!

@nknapp
Copy link
Author

nknapp commented May 8, 2017

I have created my test-package with

npm init -y
npm install --save though

in an empty directory. The package.json is

{
  "name": "test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "thought": "^1.1.2"
  }
}

@siddharthkp
Copy link
Owner

Looping @alshakero in here. Do you think we introduced a bug here: pull/26/files#diff

The results of fs.readdir do not match with a du -k

@nknapp
Copy link
Author

nknapp commented May 8, 2017 via email

@siddharthkp
Copy link
Owner

That makes sense.

We used to use du but that led to inconsistent results on different environments (let's not even get into windows). That's a bad developer experience especially if you want to use this in your CI ("works on my machine" bugs)

Honestly, between correctness and consistency, I don't know what to prefer, because, you do not control the environment in which your npm module will land (talking about libraries here)

@nknapp @alshakero What are your thoughts?

@nknapp
Copy link
Author

nknapp commented May 9, 2017 via email

@alshakero
Copy link
Collaborator

@nknapp said it perfectly. It has to do with block sizes. However, before I went for FS I compared to windows native stats. And FS delivered exactly the same results, as in ===.

So on Windows, I don't think we need to be any more accurate than Windows itself. And I took the numbers I put in fractures from Windows stats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants