Skip to content

Add simplifyFileSize option - #114

Open
KroshSputnik wants to merge 12 commits into
expressjs:masterfrom
KroshSputnik:master
Open

Add simplifyFileSize option#114
KroshSputnik wants to merge 12 commits into
expressjs:masterfrom
KroshSputnik:master

Conversation

@KroshSputnik

@KroshSputnik KroshSputnik commented Jan 29, 2025

Copy link
Copy Markdown

I added a new option to enable to show the files sizes in B, MB, GB, TB, PB.

Enable it in the options by doing
'simplifyFileSize':true

Not sure if I need to divide the size by 1024 or 1000.
Also would I need to round the final size, as it seems the size returned for the file size is already rounded.
Feedback will be appreciated.

I added a new option to enable to show the files in B, MB, GB, TB, PB. Not sure if I need to divide the size by 1024 or 1000. Feedback will be appreciated.

@UlisesGascon UlisesGascon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this proposal @KroshSputnik! Can you include tests and documentation?

@UlisesGascon

Copy link
Copy Markdown
Member

Currently we support Node@0.8 so new ES6 syntax is not allowed:

> serve-index@1.9.1 test /home/runner/work/serve-index/serve-index
> mocha --reporter spec --bail --check-leaks test/


/home/runner/work/serve-index/serve-index/index.js:308
      const fileSizes = ['B', 'KB', 'MB', 'GB', 'TB', 'PB']
      ^^^^^
SyntaxError: Use of const in strict mode.
    at Module._compile (module.js:439:25)
    at Object.Module._extensions..js (module.js:474:[10](https://github.com/expressjs/serve-index/actions/runs/13029380620/job/36428408044?pr=114#step:10:11))
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/home/runner/work/serve-index/serve-index/test/test.js:8:18)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at /home/runner/work/serve-index/serve-index/node_modules/mocha/lib/mocha.js:220:27
    at Array.forEach (native)
    at Mocha.loadFiles (/home/runner/work/serve-index/serve-index/node_modules/mocha/lib/mocha.js:217:14)
    at Mocha.run (/home/runner/work/serve-index/serve-index/node_modules/mocha/lib/mocha.js:469:10)
    at Object.<anonymous> (/home/runner/work/serve-index/serve-index/node_modules/mocha/bin/_mocha:404:18)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:[11](https://github.com/expressjs/serve-index/actions/runs/13029380620/job/36428408044?pr=114#step:10:12)9:16)
    at node.js:945:3
npm ERR! weird error 8
npm ERR! not ok code 0

Full log

Fixed the es6 issue of using `const`.
Updated readme about the new functionality
@KroshSputnik
KroshSputnik marked this pull request as draft January 31, 2025 03:12
@KroshSputnik

KroshSputnik commented Jan 31, 2025

Copy link
Copy Markdown
Author

Thanks for this proposal @KroshSputnik! Can you include tests and documentation?

Hi @UlisesGascon
I'm not sure how to add a test, do I need to create a new one in test.js?
I have never created one before, could you walk it through?

I've added docs in readme and fixed the es6 issue.

Thanks

Created tests for the new `simplifyFileSize` option.
@KroshSputnik

Copy link
Copy Markdown
Author

@UlisesGascon I have added test, for when the option is enabled and when it isnt.

Can you double check my pull request.

@KroshSputnik
KroshSputnik marked this pull request as ready for review February 1, 2025 12:27
@KroshSputnik KroshSputnik changed the title Added new feature to simplify file size. (Test & Docs Added) Added new feature to simplify file size. Feb 1, 2025
@bjohansebas

Copy link
Copy Markdown
Member

It looks great, although we could probably delegate that logic to bytes instead of handling it ourselves.

Now using the package bytes, to convert.
@socket-security

socket-security Bot commented Feb 1, 2025

Copy link
Copy Markdown

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/bytes@3.1.2 None 0 12.3 kB dougwilson

View full report↗︎

@KroshSputnik

Copy link
Copy Markdown
Author

@bjohansebas Updated the feature with your suggestion.

@bjohansebas

Copy link
Copy Markdown
Member

Great! Once the tests are run, I'll be able to give my LGTM

Added the decimal places.

@UlisesGascon UlisesGascon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for include the tests and make the changes to support Node@0.8 @KroshSputnik! :)

I added some suggestions, I think that we will need to modify the HISTORY.md to include the new dependency and the feature too 👍

Comment thread package.json Outdated
Comment thread test/test.js
@UlisesGascon UlisesGascon changed the title (Test & Docs Added) Added new feature to simplify file size. Added new feature to simplify file size. Feb 2, 2025
@UlisesGascon UlisesGascon changed the title Added new feature to simplify file size. Add simplifyFileSize option Feb 2, 2025
KroshSputnik and others added 4 commits February 2, 2025 21:22
Supporting Node@0.8 versions.

Co-authored-by: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Added another check for default config.

Co-authored-by: Ulises Gascón <ulisesgascongonzalez@gmail.com>
History about new simplifyFileSize feature and dependency of bytes.
@KroshSputnik

Copy link
Copy Markdown
Author

@UlisesGascon All tasks completed. If need anything else, let me know.

@UlisesGascon

Copy link
Copy Markdown
Member

mmm.. not sure why one of the new tests is failing 🤔. Can you have a look @KroshSputnik ?

@KroshSputnik

KroshSputnik commented Feb 3, 2025

Copy link
Copy Markdown
Author

I think the issues due to the differences of how linux and windows calculate bytes.

I downgraded to node 0.8, but all tests pass successfully.

@bjohansebas bjohansebas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, since a v2 is being planned, what if we make this the default behavior so no option has to be passed? Also, I think this should go into v2 and we should focus on that.

also for unblock this #145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants