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

chore: use "kB" everywhere with the correct definition #14061

Merged
merged 3 commits into from Aug 21, 2023

Conversation

naruaway
Copy link
Contributor

@naruaway naruaway commented Aug 10, 2023

Recently I got confused with inconsistent usages across tools around "Kilobyte".
And then I just found Vite chose to use "kB = 1000 bytes" in #10982.

This PR is just to fix small remaining inconsistencies.

I searched these using ripgrep by running rg --hidden -i -w kb and rg --hidden -i -w kib

Changes in this PR should not affect the behavior of Vite itself

@stackblitz
Copy link

stackblitz bot commented Aug 10, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@@ -329,10 +329,10 @@ function bundleSizeLimit(limit: number): Plugin {
.join(''),
'utf-8',
)
const kb = size / 1024
const kb = size / 1000
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this change triggered the limit 😅

packages/vite build: [!] (plugin bundle-limit) Error: Bundle size exceeded 120 kB, current size is 121.86kb.
packages/vite build:     at Object.generateBundle (file:///home/runner/work/vite/vite/packages/vite/rollup.config-1691641081524.mjs:360:23)
packages/vite build:     at /home/runner/work/vite/vite/node_modules/.pnpm/rollup@3.28.0/node_modules/rollup/dist/shared/rollup.js:1909:40
packages/vite build:  ELIFECYCLE  Command failed with exit code 1.
packages/vite build: ERROR: "build-bundle" exited with 1.
packages/vite build: Failed

Copy link
Contributor Author

@naruaway naruaway Aug 10, 2023

Choose a reason for hiding this comment

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

The previous limit was actually 120 kiB, which is 122880 bytes = 122.88 kB

I think 123 kB seems reasonable approximation assuming we do not want to put weird number 122.88 in the config. Now I did the change in 3b4904f

@naruaway naruaway changed the title chore: Use "kB" consitently everywhere in its correct meaning chore: use "kB" everywhere with the correct definition Aug 10, 2023
ArnaudBarre
ArnaudBarre previously approved these changes Aug 11, 2023
@sapphi-red
Copy link
Member

It seems we are using 4KiB as a default value for build.assetsInlineLimit.

/**
* Static asset files smaller than this number (in bytes) will be inlined as
* base64 strings. Default limit is `4096` (4kb). Set to `0` to disable.
* @default 4096
*/
assetsInlineLimit?: number

I guess it's better to keep the value as-is to avoid the breaking change, and correct the 4kB comment to 4KiB.

@naruaway
Copy link
Contributor Author

@sapphi-red, thanks, I missed that pattern. I now ran rg --hidden -i '\dkib', rg --hidden -i '\dkb', and rg --hidden -wi kbs to search for more patterns and made them consistent in 848f083 without breaking observable behavior of Vite

@sapphi-red sapphi-red added the p1-chore Doesn't change code behavior (priority) label Aug 19, 2023
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks!

@bluwy bluwy merged commit f97ef58 into vitejs:main Aug 21, 2023
11 checks passed
@bluwy bluwy mentioned this pull request Aug 21, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants