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.

Sign up for GitHub

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

Jump to bottom

Warn when response body is larger than 5mb #26831

Merged
merged 15 commits into from
Jul 2, 2021
Merged

Warn when response body is larger than 5mb #26831

merged 15 commits into from
Jul 2, 2021

Conversation

padmaia
Copy link
Member

@padmaia padmaia commented Jul 1, 2021 •
edited
Loading

This PR adds a warning when api responses exceed 5mb since this will end up failing once deployed. In a future version this scenario will throw an error.

Bug

  • Integration tests added

Documentation / Examples

  • Make sure the linting passes

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Jul 1, 2021
ijjk
ijjk reviewed Jul 1, 2021
View reviewed changes
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Changes look good to me, left a couple comments on the types but should be good after adding tests which we can add in test/integration/api-support 👍

padmaia and others added 2 commits July 1, 2021 13:44
@padmaia @ijjk
Apply suggestions from code review
6ea9d0c 
Co-authored-by: JJ Kasper <jj@jjsweb.site>
@padmaia
Fix types
e562d49
@ijjk

This comment has been minimized.

Sign in to view
@padmaia padmaia marked this pull request as ready for review July 1, 2021 23:24
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk

This comment has been minimized.

Sign in to view
@ijjk
Copy link
Member

ijjk commented Jul 2, 2021

Failing test suites

Commit: 39db507

test/acceptance/ReactRefreshLogBox.dev.test.js

  • server-side only compilation errors

Expand output

● server-side only compilation errors

ScriptTimeoutError: script timeout
  (Session info: headless chrome=91.0.4472.114)

  72 |
  73 |             // Wait for application to re-hydrate:
> 74 |             await browser.executeAsyncScript(function () {
     |             ^
  75 |               var callback = arguments[arguments.length - 1]
  76 |               if (window.__NEXT_HYDRATED) {
  77 |                 callback()

  at Object.throwDecodedError (../node_modules/selenium-webdriver/lib/error.js:550:15)
  at parseHttpResponse (../node_modules/selenium-webdriver/lib/http.js:565:13)
  at Executor.execute (../node_modules/selenium-webdriver/lib/http.js:491:26)
      at runMicrotasks (<anonymous>)
  at Proxy.execute (../node_modules/selenium-webdriver/lib/webdriver.js:700:17)
  at Object.patch (acceptance/helpers.js:74:13)
  at Object.<anonymous> (acceptance/ReactRefreshLogBox.dev.test.js:1415:3)

padmaia and others added 4 commits July 2, 2021 11:14
styfle
styfle previously approved these changes Jul 2, 2021
View reviewed changes
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks! 🎉

@styfle styfle requested review from ijjk and timneutkens July 2, 2021 18:30
@ijjk

This comment has been minimized.

Sign in to view
@ijjk
Copy link
Member

ijjk commented Jul 2, 2021

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
buildDuration 13.6s 13.7s ⚠️ +150ms
buildDurationCached 3.4s 3.3s -44ms
nodeModulesSize 49.3 MB 49.3 MB ⚠️ +2.02 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.356 2.387 ⚠️ +0.03
/ avg req/sec 1060.98 1047.39 ⚠️ -13.59
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.343 1.359 ⚠️ +0.02
/error-in-render avg req/sec 1861.65 1839.82 ⚠️ -21.83
Client Bundles (main, webpack, commons)
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
359.HASH.js gzip 3.09 kB 3.09 kB ✓
framework-HASH.js gzip 42 kB 42 kB ✓
main-HASH.js gzip 20.2 kB 20.2 kB ✓
webpack-HASH.js gzip 1.49 kB 1.49 kB ✓
Overall change 66.9 kB 66.9 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB ✓
Overall change 31.1 kB 31.1 kB ✓
Client Pages
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
_app-HASH.js gzip 803 B 803 B ✓
_error-HASH.js gzip 3.18 kB 3.18 kB ✓
amp-HASH.js gzip 526 B 526 B ✓
css-HASH.js gzip 329 B 329 B ✓
hooks-HASH.js gzip 903 B 903 B ✓
index-HASH.js gzip 263 B 263 B ✓
link-HASH.js gzip 1.65 kB 1.65 kB ✓
routerDirect..HASH.js gzip 322 B 322 B ✓
withRouter-HASH.js gzip 320 B 320 B ✓
bb14e60e810b..30f.css gzip 125 B 125 B ✓
Overall change 8.42 kB 8.42 kB ✓
Client Build Manifests
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
_buildManifest.js gzip 390 B 390 B ✓
Overall change 390 B 390 B ✓
Rendered Page Sizes
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
index.html gzip 523 B 523 B ✓
link.html gzip 535 B 535 B ✓
withRouter.html gzip 515 B 515 B ✓
Overall change 1.57 kB 1.57 kB ✓
Webpack 4 Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
buildDuration 12.1s 12.2s ⚠️ +173ms
buildDurationCached 4.9s 4.8s -84ms
nodeModulesSize 49.3 MB 49.3 MB ⚠️ +2.02 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.448 2.423 -0.02
/ avg req/sec 1021.25 1031.85 +10.6
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.43 1.469 ⚠️ +0.04
/error-in-render avg req/sec 1748.58 1701.93 ⚠️ -46.65
Client Bundles (main, webpack, commons)
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
14.HASH.js gzip 3.11 kB 3.11 kB ✓
677f882d2ed8..HASH.js gzip 13.4 kB 13.4 kB ✓
framework.HASH.js gzip 41.8 kB 41.8 kB ✓
main-HASH.js gzip 8.07 kB 8.07 kB ✓
webpack-HASH.js gzip 1.19 kB 1.19 kB ✓
Overall change 67.5 kB 67.5 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
polyfills-HASH.js gzip 31.3 kB 31.3 kB ✓
Overall change 31.3 kB 31.3 kB ✓
Client Pages
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
_app-HASH.js gzip 1.07 kB 1.07 kB ✓
_error-HASH.js gzip 3.83 kB 3.83 kB ✓
amp-HASH.js gzip 531 B 531 B ✓
css-HASH.js gzip 333 B 333 B ✓
hooks-HASH.js gzip 910 B 910 B ✓
index-HASH.js gzip 227 B 227 B ✓
link-HASH.js gzip 1.64 kB 1.64 kB ✓
routerDirect..HASH.js gzip 295 B 295 B ✓
withRouter-HASH.js gzip 292 B 292 B ✓
e025d2764813..52f.css gzip 125 B 125 B ✓
Overall change 9.26 kB 9.26 kB ✓
Client Build Manifests
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
_buildManifest.js gzip 418 B 418 B ✓
Overall change 418 B 418 B ✓
Rendered Page Sizes
vercel/next.js canary padmaia/next.js fix/warn-sending-5mb Change
index.html gzip 567 B 567 B ✓
link.html gzip 580 B 580 B ✓
withRouter.html gzip 559 B 559 B ✓
Overall change 1.71 kB 1.71 kB ✓
Commit: 11d42e9

ijjk
ijjk approved these changes Jul 2, 2021
View reviewed changes
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@kodiakhq kodiakhq bot merged commit 538095c into vercel:canary Jul 2, 2021
@padmaia padmaia deleted the fix/warn-sending-5mb branch July 2, 2021 19:59
@padmaia padmaia mentioned this pull request Jul 2, 2021
Merged
kodiakhq bot pushed a commit that referenced this pull request Jul 2, 2021
@padmaia
5MB -> 4MB body size limit (#26887)
b8f3a6d 
This decreases the body size limit that triggers a warning from 5MB -> 4MB, which provides a little more wiggle room. Certain things like using base64 on body, headers, path, etc can cause the response to be larger than initially calculated. 

Initial PR: #26831
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
@padmaia
Warn when response body is larger than 5mb (vercel#26831)
5df335d 
This PR adds a warning when api responses exceed 5mb since this will end up failing once deployed. In a future version this scenario will throw an error.

## Bug

- [x] Integration tests added

## Documentation / Examples

- [x] Make sure the linting passes
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
@padmaia
5MB -> 4MB body size limit (vercel#26887)
718342e 
This decreases the body size limit that triggers a warning from 5MB -> 4MB, which provides a little more wiggle room. Certain things like using base64 on body, headers, path, etc can cause the response to be larger than initially calculated. 

Initial PR: vercel#26831
@700software 700software mentioned this pull request Jan 10, 2022
Closed
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ijjk ijjk ijjk approved these changes

@styfle styfle styfle approved these changes

@huozhi huozhi Awaiting requested review from huozhi

@shuding shuding Awaiting requested review from shuding

@timneutkens timneutkens Awaiting requested review from timneutkens

Assignees
No one assigned
Labels
created-by: Next.js team PRs by the Next.js team. type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@padmaia @ijjk @styfle @timneutkens