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

Skip lock file parsing if it's larger than 10MB #1140

Conversation

codykaup
Copy link
Contributor

@codykaup codykaup commented Jan 10, 2025

We've been seeing some OOM errors in TurboSnap that appears to come from our lock file parser. This attempts to alleviate that issue by completely skipping the lock file parsing if it's larger than 10 MB so we still get some of the benefits of TurboSnap without completely disabling it.

In case it wasn't obvious, the 10MB limit is a guess. We can adjust it as needed.

📦 Published PR as canary version: 11.24.0--canary.1140.12756027021.0

✨ Test out this PR locally via:

npm install [email protected]
# or 
yarn add [email protected]

@codykaup codykaup added release Auto: Create a `latest` release when merged minor Auto: Increment the minor version when merged labels Jan 10, 2025
@codykaup codykaup self-assigned this Jan 10, 2025
Copy link
Contributor

github-actions bot commented Jan 10, 2025

📦 Package Size: 5448 KB

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.44%. Comparing base (a2bbb87) to head (6c21e16).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1140      +/-   ##
==========================================
+ Coverage   69.39%   69.44%   +0.05%     
==========================================
  Files         202      202              
  Lines        7322     7335      +13     
  Branches     1280     1284       +4     
==========================================
+ Hits         5081     5094      +13     
  Misses       2218     2218              
  Partials       23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codykaup codykaup marked this pull request as ready for review January 10, 2025 21:58
@codykaup codykaup requested review from ethriel3695 and a team January 10, 2025 21:58
Copy link
Contributor

@ethriel3695 ethriel3695 left a comment

Choose a reason for hiding this comment

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

Looks great, @codykaup !

@ethriel3695
Copy link
Contributor

@codykaup One question on this.
We talked about adding logs to see if this feature is working.
Should we add a sentry log to this line where it doesn't get the dependencies?
https://github.com/chromaui/chromatic-cli/pull/1140/files#diff-155aeba584d040ebd2838086800bf6684e93811b14756b9e84d0f73427bfdd0eL32

Copy link
Contributor

@jmhobbs jmhobbs left a comment

Choose a reason for hiding this comment

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

Did we want to expose this via environment variable as well, in case it's running on smaller CI boxes and still OOM's at 10MB?

@codykaup
Copy link
Contributor Author

@codykaup One question on this. We talked about adding logs to see if this feature is working. Should we add a sentry log to this line where it doesn't get the dependencies? https://github.com/chromaui/chromatic-cli/pull/1140/files#diff-155aeba584d040ebd2838086800bf6684e93811b14756b9e84d0f73427bfdd0eL32

@ethriel3695 that's a separate ticket. I wanted to fix the issue quickly then follow it up with monitoring. Good callout though!

Did we want to expose this via environment variable as well, in case it's running on smaller CI boxes and still OOM's at 10MB

Yeah probably. I'll toss that in here.

@codykaup codykaup requested a review from jmhobbs January 13, 2025 17:55
@codykaup codykaup added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit 279132b Jan 13, 2025
21 checks passed
@codykaup codykaup deleted the cody/cap-2634-skip-lock-file-parsing-if-the-manifest-is-larger-than-10mb branch January 13, 2025 21:01
@chromatic-ci-bot
Copy link
Collaborator

🚀 PR was released in v11.23.0 🚀

@chromatic-ci-bot chromatic-ci-bot added the released Verdict: This issue/pull request has been released label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Auto: Increment the minor version when merged release Auto: Create a `latest` release when merged released Verdict: This issue/pull request has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants