Skip to content

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

Merged
bluwy merged 3 commits intovitejs:mainfrom
naruaway:consistent-kb
Aug 21, 2023
Merged

chore: use "kB" everywhere with the correct definition#14061
bluwy merged 3 commits intovitejs:mainfrom
naruaway:consistent-kb

Conversation

@naruaway
Copy link
Copy Markdown
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

@bolt-new-by-stackblitz
Copy link
Copy Markdown

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

'utf-8',
)
const kb = size / 1024
const kb = size / 1000
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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.

4 participants