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

Add flag to disable API warning #11380

Merged
merged 2 commits into from
May 7, 2020
Merged

Conversation

PabloSzx
Copy link
Contributor

@PabloSzx PabloSzx commented Mar 26, 2020

This flag is useful when you are using an external API resolver like express when defining an API route, since the native functionality doesn't realize that the API actually sent a response.

A very simple use case example https://github.com/PabloSzx/next-external-api-resolver-example

@ijjk
Copy link
Member

ijjk commented Mar 26, 2020

Stats from current PR

Default Server Mode
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 11.2s 11.2s ⚠️ +15ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Rendered Page Sizes
zeit/next.js canary PabloSzx/next.js externalResolver Change
index.html gzip 917 B 917 B
link.html gzip 925 B 925 B
withRouter.html gzip 915 B 915 B
Overall change 2.76 kB 2.76 kB

Serverless Mode (Decrease detected ✓)
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 12.2s 12s -193ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Serverless bundles Overall decrease ✓
zeit/next.js canary PabloSzx/next.js externalResolver Change
_error.js gzip 294 kB 293 kB -358 B
404.html gzip 1.32 kB 1.32 kB
hooks.html gzip 958 B 958 B
index.js gzip 294 kB 293 kB -667 B
link.js gzip 301 kB 302 kB ⚠️ +420 B
routerDirect.js gzip 300 kB 300 kB ⚠️ +367 B
withRouter.js gzip 300 kB 300 kB ⚠️ +87 B
Overall change 1.49 MB 1.49 MB -151 B

@Janpot
Copy link
Contributor

Janpot commented Mar 26, 2020

I'd remove this warning altogether.

@ijjk
Copy link
Member

ijjk commented Mar 26, 2020

Stats from current PR

Default Server Mode
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 10.7s 10.6s -57ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Rendered Page Sizes
zeit/next.js canary PabloSzx/next.js externalResolver Change
index.html gzip 917 B 917 B
link.html gzip 925 B 925 B
withRouter.html gzip 915 B 915 B
Overall change 2.76 kB 2.76 kB

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 11.4s 11.8s ⚠️ +429ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Serverless bundles Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
_error.js gzip 293 kB 294 kB ⚠️ +1.34 kB
404.html gzip 1.32 kB 1.32 kB
hooks.html gzip 958 B 958 B
index.js gzip 294 kB 295 kB ⚠️ +620 B
link.js gzip 302 kB 302 kB ⚠️ +133 B
routerDirect.js gzip 300 kB 300 kB -71 B
withRouter.js gzip 300 kB 300 kB -245 B
Overall change 1.49 MB 1.49 MB ⚠️ +1.78 kB

@ijjk
Copy link
Member

ijjk commented Mar 26, 2020

Stats from current PR

Default Server Mode
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 9.3s 9.8s ⚠️ +500ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Rendered Page Sizes
zeit/next.js canary PabloSzx/next.js externalResolver Change
index.html gzip 917 B 917 B
link.html gzip 925 B 925 B
withRouter.html gzip 915 B 915 B
Overall change 2.76 kB 2.76 kB

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 10.6s 11s ⚠️ +365ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Serverless bundles Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
_error.js gzip 294 kB 294 kB -5 B
404.html gzip 1.32 kB 1.32 kB
hooks.html gzip 958 B 958 B
index.js gzip 294 kB 295 kB ⚠️ +749 B
link.js gzip 302 kB 302 kB -4 B
routerDirect.js gzip 300 kB 300 kB ⚠️ +74 B
withRouter.js gzip 300 kB 300 kB ⚠️ +54 B
Overall change 1.49 MB 1.49 MB ⚠️ +868 B

@ijjk
Copy link
Member

ijjk commented Mar 26, 2020

Stats from current PR

Default Server Mode
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 9.2s 9.5s ⚠️ +357ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Rendered Page Sizes
zeit/next.js canary PabloSzx/next.js externalResolver Change
index.html gzip 917 B 917 B
link.html gzip 925 B 925 B
withRouter.html gzip 915 B 915 B
Overall change 2.76 kB 2.76 kB

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
buildDuration 10.4s 10.3s -107ms
nodeModulesSize 53.1 MB 53.1 MB ⚠️ +544 B
Client Bundles (main, webpack, commons)
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.js gzip 6.24 kB 6.24 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..c6c1.js gzip 10.1 kB 10.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 56.2 kB 56.2 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
main-HASH.module.js gzip 4.77 kB 4.77 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.71 kB 6.71 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary PabloSzx/next.js externalResolver Change
polyfills-HASH.js gzip 26.3 kB 26.3 kB
Overall change 26.3 kB 26.3 kB
Client Pages
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.js gzip 1.24 kB 1.24 kB
_error.js gzip 3.15 kB 3.15 kB
hooks.js gzip 664 B 664 B
index.js gzip 222 B 222 B
link.js gzip 2.03 kB 2.03 kB
routerDirect.js gzip 279 B 279 B
withRouter.js gzip 278 B 278 B
Overall change 7.86 kB 7.86 kB
Client Pages Modern
zeit/next.js canary PabloSzx/next.js externalResolver Change
_app.module.js gzip 594 B 594 B
_error.module.js gzip 2.08 kB 2.08 kB
hooks.module.js gzip 370 B 370 B
index.module.js gzip 212 B 212 B
link.module.js gzip 1.48 kB 1.48 kB
routerDirect..dule.js gzip 271 B 271 B
withRouter.m..dule.js gzip 270 B 270 B
Overall change 5.28 kB 5.28 kB
Client Build Manifests
zeit/next.js canary PabloSzx/next.js externalResolver Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Serverless bundles Overall increase ⚠️
zeit/next.js canary PabloSzx/next.js externalResolver Change
_error.js gzip 293 kB 294 kB ⚠️ +392 B
404.html gzip 1.32 kB 1.32 kB
hooks.html gzip 958 B 958 B
index.js gzip 293 kB 294 kB ⚠️ +593 B
link.js gzip 302 kB 302 kB ⚠️ +186 B
routerDirect.js gzip 300 kB 300 kB ⚠️ +137 B
withRouter.js gzip 300 kB 300 kB ⚠️ +276 B
Overall change 1.49 MB 1.49 MB ⚠️ +1.58 kB

@timneutkens
Copy link
Member

Overall it'd be better to figure out why the warning is triggered in the first place.

I'd remove this warning altogether.

Considering we saw users getting their API routes stuck without resolving (causing quite a lot of cost for them) I'm strongly 👎 on removing the warning. We should just figure out in what cases it's not actionable and fix those.

@PabloSzx
Copy link
Contributor Author

PabloSzx commented Mar 27, 2020

Overall it'd be better to figure out why the warning is triggered in the first place.

The problem is that when you are using express or connect for example, the res.finished or res.headersSent may not be used, but the request itself was correctly handled, so isResSent() wil always be false no matter what, and this warning in these specific cases is just making noise, right now to ignore this warning and not flood your console you have to use something like ignore-warnings or make a custom function that suppreses this specific warning.

EDIT: express does use headersSent, but the callback style for the middlewares is the main reason the warning is triggered

@timneutkens
Copy link
Member

Wondering if you can detect express usage

@Janpot
Copy link
Contributor

Janpot commented Mar 27, 2020

Even a trivial handler like:

export default async function (req, res) {
  setTimeout(() => {
    res.send('hello');
  }, 1000);
}

triggers the warning. Which is pretty common code style for node code. I think it's impossible to correctly detect stalled requests with this signature of API handlers. A lot of the existing middleware is not explicitly written for express, so would be impossible to detect.

With the risk of going a bit on a tangent here, one thing I could suggest is to start rethinking the next.js API feature a bit. To make it a bit more aligned with the new GSSP methods, and make it work better with the lambda function style than express/connect handlers do. Maybe something like:

// ./pages/api2/my-api.js
export default async function ({ env, req, res }) {
  return {
    data: {...}
  }
}

This sort of signature would make the need of this warning obsolete as this would always return a value for the response body.

Or even go a lot further and think about abstracting the whole HTTP layer away in a sort of rpc mechanism and allow importing API methods both serverside and clientside.

// ./pages/rpc/my-rpc.js

// when imported serverside this gets executed directly
// when imported clientside this gets executed through a fetch
export async function myMethod (params) {
  return {...}
}

@PabloSzx
Copy link
Contributor Author

Even a trivial handler like:

export default async function (req, res) {
  setTimeout(() => {
    res.send('hello');
  }, 1000);
}

triggers the warning. Which is pretty common code style for node code. I think it's impossible to correctly detect stalled requests with this signature of API handlers. A lot of the existing middleware is not explicitly written for express, so would be impossible to detect.

I think that is the main reason the warning is being triggered is because of this, express (and all middlewares) don't work as an async function, but using the next() function, which is the callback style, not async -> await | then

With the risk of going a bit on a tangent here, one thing I could suggest is to start rethinking the next.js API feature a bit. To make it a bit more aligned with the new GSSP methods, and make it work better with the lambda function style than express/connect handlers do. Maybe something like:

// ./pages/api2/my-api.js
export default async function ({ env, req, res }) {
  return {
    data: {...}
  }
}

This sort of signature would make the need of this warning obsolete as this would always return a value for the response body.

Or even go a lot further and think about abstracting the whole HTTP layer away in a sort of rpc mechanism and allow importing API methods both serverside and clientside.

// ./pages/rpc/my-rpc.js

// when imported serverside this gets executed directly
// when imported clientside this gets executed through a fetch
export async function myMethod (params) {
  return {...}
}

I think this way you are not only "improving" the API functionality (with ton of breaking changes) but also killing in the way all possible out of the box useful middlewares out there for the Node.js ecosystem.

@timneutkens
Copy link
Member

I think that is the main reason the warning is being triggered is because of this, express (and all middlewares) don't work as an async function, but using the next() function, which is the callback style, not async -> await | then

Fair observation, maybe we should only add the warning when the method does not return a promise (is not async) 🤔

@PabloSzx
Copy link
Contributor Author

PabloSzx commented Mar 27, 2020

I think that is the main reason the warning is being triggered is because of this, express (and all middlewares) don't work as an async function, but using the next() function, which is the callback style, not async -> await | then

Fair observation, maybe we should only add the warning when the method does not return a promise (is not async) 🤔

Something like this? 🤔

    // Call API route method
    const resolverResult = resolver(req, res)

    if (resolverResult && typeof resolverResult.then === 'function') {
      await resolverResult
      if (
        process.env.NODE_ENV !== 'production' &&
        !isResSent(res) &&
        !wasPiped
      ) {
        console.warn(
          `API resolved without sending a response for ${req.url}, this may result in stalled requests.`
        )
      }
    }

EDIT: I realized that you said the opposite, well, then this flag to manually turn off this warning still would be useful.

@Janpot
Copy link
Contributor

Janpot commented Mar 27, 2020

Fair observation, maybe we should only add the warning when the method does not return a promise (is not async)

Wouldn't that make

export default function (req, res) {
  setTimeout(() => {
    res.send('hello');
  }, 1000);
}

still throw the warning? i.e. the method is not async, and res.send is not called before it returns.

FYI: Also suggested another approach here #10589 (comment) but not sure that would cover the intent of the warning neither. Perhaps just adding the warning when the handler returns a promise would make sense, but I also think it would still fail to catch a lot of cases that it intends to protect against.


off-topic:

I think this way you are not only "improving" the API functionality (with ton of breaking changes) but also killing in the way all possible out of the box useful middlewares out there for the Node.js ecosystem.

The goal was just to attempt to open the minds to iterate and innovate on the current state of the library. I don't suggest implementing exactly this, neither do I suggest to implement things in a breaking way. Also, a different signature of API handler doesn't make all existing middleware obsolete, as long as IncomingMessage and ServerResponse are used, writing an adapter from one signature to the other would be trivial. Nota bene: an adapter that you need to already write if you want to use existing middleware with GSSP.

Copy link
Member

@Timer Timer left a comment

Choose a reason for hiding this comment

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

Closing per PR discussion: we're currently more interested in fixing false positives than allowing the warning to be disabled.

The warning doesn't fail the build, and only triggers for exceptional use cases.

Thanks!

@Timer Timer closed this May 2, 2020
@timneutkens timneutkens reopened this May 6, 2020
@timneutkens
Copy link
Member

I'm not opposed to landing it if it helps people. This PR does need tests though. Please add them to test/integration 🙏

@PabloSzx PabloSzx force-pushed the externalResolver branch from 77b1536 to 575d284 Compare May 7, 2020 04:52
This flag is useful when you are using an external API resolver like express when defining an API route, since the native functionality doesn't realize that the API actually sent a response.

A very simple use case example https://github.com/PabloSzx/next-external-api-resolver-example

fixes vercel#10589
@PabloSzx PabloSzx force-pushed the externalResolver branch from 575d284 to 079b4af Compare May 7, 2020 05:00
@PabloSzx
Copy link
Contributor Author

PabloSzx commented May 7, 2020

This PR does need tests though. Please add them to test/integration 🙏

Done 👌

Copy link
Member

@timneutkens timneutkens 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 to me, @ijjk could you review the tests just to be sure.

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.

Added tests look good to me

@timneutkens timneutkens merged commit dac715e into vercel:canary May 7, 2020
rokinsky pushed a commit to rokinsky/next.js that referenced this pull request Jul 11, 2020
* Add flag to disable API warning

This flag is useful when you are using an external API resolver like express when defining an API route, since the native functionality doesn't realize that the API actually sent a response.

A very simple use case example https://github.com/PabloSzx/next-external-api-resolver-example

fixes vercel#10589

* Update api-middlewares.md

Co-authored-by: Tim Neutkens <[email protected]>
@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants