Update middleware eval checks #30883

javivelasco · 2021-11-03T12:01:12Z

Co-authored-by: Tobias Koppers sokra@users.noreply.github.com

With this PR we are updating the way we check the usage of eval and other dynamic code evaluation (like new Function) for middleware. Now instead of simply showing a warning it will behave differently depending on if we are building or in development.

Development: we replace the dynamic code with a wrapper so that we print a warning only when the code is used. We don't fail in this scenario as it is possible that once the application is built the code that uses eval is left out.
Build: we detect with tree shaking if the code that will be bundled into the middleware includes any dynamic code and in such scenario we make the build fail as don't want to allow it for the production environment.

Closes #30674

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #number
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes by running yarn lint

styfle

Please add a test

styfle · 2021-11-04T18:51:41Z

packages/next/build/webpack/plugins/middleware-plugin.ts

@@ -39,7 +39,7 @@ export default class MiddlewarePlugin {
  }

  createAssets(
-    compilation: any,
+    compilation: webpack5.Compilation,


I think this was recently changed to any but I don't remember why

cc @timneutkens @sokra

styfle

Thanks!

test/integration/middleware/with-eval/test/index.test.js

packages/next/server/web/sandbox/sandbox.ts

Co-authored-by: Tobias Koppers <sokra@users.noreply.github.com>

styfle · 2021-11-05T13:00:38Z

packages/next/server/dev/next-dev-server.ts

@@ -647,7 +657,9 @@ export default class DevServer extends Server {
    }

    if (!usedOriginalStack) {
-      if (type) {
+      if (type === 'warning') {
+        Log.warn(err + '')


Why no "warning" prefix?

@styfle The prefix is added by Log. The one that is interpolated in the line below is for unhandledRejection and uncaughtException. The message will show up prefixed.

ijjk

Looks good, thanks!

ijjk · 2021-11-05T20:35:25Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
buildDuration	18.7s	18.7s	⚠️ +1ms
buildDurationCached	3.7s	3.8s	⚠️ +116ms
nodeModulesSize	332 MB	332 MB	⚠️ +9.76 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.071	2.977	-0.09
/ avg req/sec	814.06	839.75	+25.69
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.389	1.403	⚠️ +0.01
/error-in-render avg req/sec	1799.56	1782.49	⚠️ -17.07

Client Bundles (main, webpack, commons)

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	28 kB	28 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	71.9 kB	71.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
_app-HASH.js gzip	1.23 kB	1.23 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	635 B	635 B	✓
image-HASH.js gzip	4.44 kB	4.44 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.87 kB	1.87 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
334f979574ae..6f4.css gzip	106 B	106 B	✓
Overall change	13.1 kB	13.1 kB	✓

Client Build Manifests

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
index.html gzip	522 B	522 B	✓
link.html gzip	535 B	535 B	✓
withRouter.html gzip	515 B	515 B	✓
Overall change	1.57 kB	1.57 kB	✓

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
buildDuration	19.7s	20.1s	⚠️ +396ms
buildDurationCached	3.8s	3.8s	⚠️ +61ms
nodeModulesSize	332 MB	332 MB	⚠️ +9.76 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.938	2.988	⚠️ +0.05
/ avg req/sec	850.92	836.81	⚠️ -14.11
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.46	1.419	-0.04
/error-in-render avg req/sec	1712.07	1762.01	+49.94

Client Bundles (main, webpack, commons)

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.2 kB	28.2 kB	✓
webpack-HASH.js gzip	1.43 kB	1.43 kB	✓
Overall change	72.1 kB	72.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
_app-HASH.js gzip	1.22 kB	1.22 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	622 B	622 B	✓
image-HASH.js gzip	4.46 kB	4.46 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	1.91 kB	1.91 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
334f979574ae..6f4.css gzip	106 B	106 B	✓
Overall change	13.1 kB	13.1 kB	✓

Client Build Manifests

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	javivelasco/next.js update-eval-checks	Change
index.html gzip	521 B	521 B	✓
link.html gzip	534 B	534 B	✓
withRouter.html gzip	515 B	515 B	✓
Overall change	1.57 kB	1.57 kB	✓

Commit: 21a04d9

javivelasco requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners November 3, 2021 12:01

ijjk added the type: next label Nov 3, 2021

javivelasco force-pushed the update-eval-checks branch from 9286ad3 to 84ed339 Compare November 3, 2021 12:01

This comment has been minimized.

Sign in to view

styfle reviewed Nov 3, 2021

View reviewed changes

javivelasco force-pushed the update-eval-checks branch from 84ed339 to c43d311 Compare November 4, 2021 18:30

javivelasco requested a review from styfle November 4, 2021 18:30

This comment has been minimized.

Sign in to view

styfle reviewed Nov 4, 2021

View reviewed changes

styfle previously approved these changes Nov 4, 2021

View reviewed changes

ijjk reviewed Nov 4, 2021

View reviewed changes

test/integration/middleware/with-eval/test/index.test.js Show resolved Hide resolved

ijjk reviewed Nov 4, 2021

View reviewed changes

test/integration/middleware/with-eval/test/index.test.js Show resolved Hide resolved

ijjk reviewed Nov 4, 2021

View reviewed changes

packages/next/server/web/sandbox/sandbox.ts Outdated Show resolved Hide resolved

javivelasco and others added 5 commits November 5, 2021 11:37

Update eval checks

fbf3f94

Co-authored-by: Tobias Koppers <sokra@users.noreply.github.com>

Move middleware tests

00c6c42

Add eval test

be2f558

Add tree-shaking test with eval

1138e71

Improve errors for dev and tests

053bbe5

Co-authored-by: Tobias Koppers <sokra@users.noreply.github.com>

javivelasco dismissed styfle’s stale review via 053bbe5 November 5, 2021 10:38

javivelasco force-pushed the update-eval-checks branch from c43d311 to 053bbe5 Compare November 5, 2021 10:38

javivelasco requested review from ijjk and styfle November 5, 2021 10:38

This comment has been minimized.

Sign in to view

styfle reviewed Nov 5, 2021

View reviewed changes

Merge branch 'canary' into update-eval-checks

df5c5e2

This comment has been minimized.

Sign in to view

ijjk approved these changes Nov 5, 2021

View reviewed changes

Merge branch 'canary' into update-eval-checks

21a04d9

kodiakhq bot merged commit 6e081e1 into vercel:canary Nov 5, 2021

javivelasco deleted the update-eval-checks branch November 12, 2021 12:00

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update middleware eval checks #30883

Update middleware eval checks #30883

javivelasco commented Nov 3, 2021

This comment has been minimized.

styfle left a comment

This comment has been minimized.

styfle Nov 4, 2021

styfle left a comment

This comment has been minimized.

styfle Nov 5, 2021

javivelasco Nov 5, 2021

This comment has been minimized.

ijjk left a comment •

edited

ijjk commented Nov 5, 2021

Update middleware eval checks #30883

Update middleware eval checks #30883

Conversation

javivelasco commented Nov 3, 2021

Bug

Feature

Documentation / Examples

This comment has been minimized.

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

styfle Nov 4, 2021

Choose a reason for hiding this comment

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

styfle Nov 5, 2021

Choose a reason for hiding this comment

javivelasco Nov 5, 2021

Choose a reason for hiding this comment

This comment has been minimized.

ijjk left a comment • edited

Choose a reason for hiding this comment

ijjk commented Nov 5, 2021

Stats from current PR

ijjk left a comment •

edited