Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

test: add bundle size test #8133

Merged
merged 10 commits into from Oct 19, 2022
Merged

test: add bundle size test #8133

merged 10 commits into from Oct 19, 2022

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • 🧹 Chore
  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This adds a test to constraint bundle size growth for an out-of-the-box install. This will prompt us to update snapshots when/if new packages are added to build.

Any ideas for improving assertion or test more than welcome.

I've also fixed an issue where vue-router and @vue/devtools-api were traced as dependencies even without router integration.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added chore test 🧹 p1-chore Priority 1: no change in change code behavior labels Oct 12, 2022
@danielroe danielroe requested a review from antfu October 12, 2022 14:39
@danielroe danielroe self-assigned this Oct 12, 2022
@codesandbox
Copy link

codesandbox bot commented Oct 12, 2022

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify
Copy link

netlify bot commented Oct 12, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit 8fea686
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/6350153b5f970800081bbdd3

@pi0
Copy link
Member

pi0 commented Oct 12, 2022

I've also fixed an issue where vue-router and @vue/devtools-api were traced as dependencies even without router integration.

Nice find! Do you mind splitting this into a new fix/perf PR?

I have some concerns about how we do bundle/size testing. It made honestly lots of trouble over time in Nuxt 2 and made the CI of Nuxt 2 fragile as each package can actually affect bundle size easily and hardcoded size was not a good idea. BTW I'm fine with making test limited to testing used node_modules in snapshot as the list changes much less often.

@danielroe
Copy link
Member Author

Done. But for what it's worth, I think it is worth keeping a bundle size test. It takes 10s to update the snapshots for the bundle size, and at least that means that we are deliberately choosing to increase server or client bundle size rather than doing it accidentally.

@pi0
Copy link
Member

pi0 commented Oct 15, 2022

True but it quite often happens because an (expected as we test) external library simply adds new functionality and it adds a few bytes to the server or app bundle. Over time all libs and our internal codebase increase and it is by itself a deliberate choice of us or 3rd party library authors that make this choice. Bein required to manually update the test, is a duplicate effort unless it often would cause us to revise even if using/upgrading a library or not.

I think the entire logic for size testing, could be an informational CI action similar to code coverage integration signaling us about certain percentage increase. There might be even some ready to use integrations...

In the meantime i appreciate if we remove size cap logic from PR changes completely to merge and add it back as CI integration later.

@danielroe
Copy link
Member Author

The logic is already removed unless running manually locally for ease of testing. 😊

@pi0
Copy link
Member

pi0 commented Oct 15, 2022

Size caps are still hardcoded though... Can we write sizes to (a gitginored file) for local testing? Later CI can pick same file as well and show in gh PRs.

Copy link
Member Author

I agree, it would be nice to print results in CI.

@pi0
Copy link
Member

pi0 commented Oct 19, 2022

Interestingly windows output is bigger than expected...

image

@danielroe
Copy link
Member Author

When originally pushed it was not.

@pi0
Copy link
Member

pi0 commented Oct 19, 2022

I've enabled back CI checks with hardcoded sizes. Let's hope with Nuxt 3 we require to updated the caps less often. Also let's track windows issue later.

@pi0
Copy link
Member

pi0 commented Oct 19, 2022

When originally pushed it was not.

Or it was but didn't reach the cap size. something else could added few bytes and reveal it. Also note that I've changed the measurement to use actual byte size. It is different from how much filesystem of OS takes to store files to the disk.

@pi0 pi0 merged commit e79d72c into main Oct 19, 2022
@pi0 pi0 deleted the test/bundle-size branch October 19, 2022 15:29
Copy link
Member Author

I saw that change. But the variability was not so high. The numbers were:

    expect(serverBundle.size).toBeLessThan(120000)
    // expect(serverBundle.size).toMatchInlineSnapshot('114018')

    expect(modules.size).toBeLessThan(2700000)
    // expect(modules.size).toMatchInlineSnapshot('2637251')

Whereas the difference in your last example is more like 250kB, which is a huge difference!

@pi0
Copy link
Member

pi0 commented Oct 19, 2022

Yeah that's strange! Specially that only windows is bigger. We could make different values between windows and linux for server bundle size....

Copy link
Member Author

Is it caused by reading byte size? Maybe this is the difference? Why should the size on disk have been the same, but byte size differ so much?

@pi0
Copy link
Member

pi0 commented Oct 19, 2022

I'm not suggesting it necessary was the reason. I think since then we also migrated to pnpm and upgraded couple of dependencies.

Anyway since byte size difference now, i think something is really wrong probably with nitro externals in windows (not tests) 😬

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x chore 🧹 p1-chore Priority 1: no change in change code behavior test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants