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

refactor(word): reduce definitions to 1000 in all locales #2751

Merged
merged 3 commits into from
Mar 16, 2024

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Mar 15, 2024

Rationale

Follow-on to #2265

Reduced to 1000 entries max in the words module definitions in all locales.

I selected this module because it had the biggest impact on bundle size (e.g. the hu noun file alone is approx 144KB, 10K rows.) while still being relatively easy to review.

This is a stepping stone to enabling the normalizeLocaleFile for all files (previously I noted that I didn't think it was a good idea to try to enable that for all modules at once, as it introduces massive diffs which are impossible to review, and we might accidentally lose useful information, so currently it's only enabled for new modules like food but disabled for existing modules).

Even though we dont generally consider editing definitions as semver-major, we may as well make some bundle size improvements in v9 when data is changing anyway as a result of the 53 bit randomizer #2357

Methodology

For any definitions in word module with >1000 entries, I shuffle the array, select 1000 at random and then sort (the sorting is why you see some green in the diffs and not just red)

In total this removes about 530KB from the full bundle.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.50%. Comparing base (b6b18d7) to head (aa26832).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2751      +/-   ##
==========================================
- Coverage   99.57%   99.50%   -0.08%     
==========================================
  Files        2983     2983              
  Lines      250340   215303   -35037     
  Branches      981      984       +3     
==========================================
- Hits       249269   214230   -35039     
+ Misses       1071     1044      -27     
- Partials        0       29      +29     
Files Coverage Δ
src/locales/de/word/adjective.ts 100.00% <ø> (ø)
src/locales/de/word/adverb.ts 100.00% <100.00%> (ø)
src/locales/de/word/verb.ts 100.00% <ø> (ø)
src/locales/en/word/adjective.ts 100.00% <100.00%> (ø)
src/locales/en/word/noun.ts 100.00% <ø> (ø)
src/locales/en/word/verb.ts 100.00% <ø> (ø)
src/locales/fr/word/verb.ts 100.00% <100.00%> (ø)
src/locales/hu/word/adjective.ts 100.00% <ø> (ø)
src/locales/hu/word/adverb.ts 100.00% <100.00%> (ø)
src/locales/hu/word/noun.ts 100.00% <ø> (ø)
... and 1 more

... and 30 files with indirect coverage changes

@matthewmayer matthewmayer marked this pull request as ready for review March 15, 2024 02:06
@matthewmayer matthewmayer requested a review from a team as a code owner March 15, 2024 02:06
@matthewmayer matthewmayer self-assigned this Mar 15, 2024
@matthewmayer matthewmayer added the m: word Something is referring to the word module label Mar 15, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Mar 15, 2024

Please also enable the script cleanup for the word module here:

@ST-DDT ST-DDT added this to the vAnytime milestone Mar 15, 2024
@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: locale Permutes locale definitions labels Mar 15, 2024
@matthewmayer
Copy link
Contributor Author

That would cause all other locales to be re-sorted so I will do that in a subsequent PR to avoid making the diff too big.

@ST-DDT ST-DDT requested review from a team March 15, 2024 13:24
Copy link

netlify bot commented Mar 16, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit aa26832
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/65f554a758b8b80008ec6ad5
😎 Deploy Preview https://deploy-preview-2751.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ST-DDT ST-DDT merged commit cefc1e9 into faker-js:next Mar 16, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: word Something is referring to the word module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants