Refactor FS references in the Base Server #32179

shuding · 2021-12-06T16:11:27Z

After #32154, we are moving FS related methods out of the base server and into the node server as planned in #31506. This PR removes the direct fs references.

Some method names might need to be modified in the future.

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

karaggeorge · 2021-12-06T16:32:05Z

packages/next/server/base-server.ts

+  protected abstract getPagesManifest(): PagesManifest | undefined
+  protected abstract getBuildId(): string
+  protected abstract generatePublicRoutes(): Route[]
+  protected abstract getFilesystemPaths(): Set<string>


What do we think about trying to make these async when possible? I know that we can have some of this pre-calculated, but I feel like if it's easy during rewrite, making them async is just more flexible later on

Yeah I think we can totally do that in an upcoming PR. Would be good to do the same for require too!

karaggeorge

LGTM

huozhi · 2021-12-06T22:06:54Z

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

@@ -700,10 +700,18 @@ export default class DevServer extends Server {
    })
  }

+  protected getPagesManifest(): undefined {


is getPagesManifest still required to be defined since it derives from the next-server?
same with getMiddlewareManifest

Yes since I refactored the caller conditions in the base server a bit so they'are always called: https://github.com/vercel/next.js/pull/32179/files?diff=unified&w=0#diff-2b8e3503c1bebf856610a5551d9c5996435b99f45db389d4eeac1b671518875eL306-R310

Goal is to eventually get rid of ad-hoc dev server logic from the base server.

ijjk · 2021-12-06T22:59:59Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	shuding/next.js shu/9f14	Change
buildDuration	18.1s	17.9s	-181ms
buildDurationCached	3.3s	3.3s	-17ms
nodeModulesSize	347 MB	347 MB	⚠️ +2.14 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	shuding/next.js shu/9f14	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.961	2.901	-0.06
/ avg req/sec	844.42	861.83	+17.41
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.358	1.364	⚠️ +0.01
/error-in-render avg req/sec	1840.93	1832.46	⚠️ -8.47

Client Bundles (main, webpack, commons)

	vercel/next.js canary	shuding/next.js shu/9f14	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	28.4 kB	28.4 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.3 kB	72.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	shuding/next.js shu/9f14	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	shuding/next.js shu/9f14	Change
_app-HASH.js gzip	1.37 kB	1.37 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 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.49 kB	4.49 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	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	13.3 kB	13.3 kB	✓

Client Build Manifests

	vercel/next.js canary	shuding/next.js shu/9f14	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes

	vercel/next.js canary	shuding/next.js shu/9f14	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC

General Overall increase ⚠️

	vercel/next.js canary	shuding/next.js shu/9f14	Change
buildDuration	19.4s	19.4s	⚠️ +38ms
buildDurationCached	3.3s	3.3s	-45ms
nodeModulesSize	347 MB	347 MB	⚠️ +2.14 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	shuding/next.js shu/9f14	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.959	2.932	-0.03
/ avg req/sec	844.81	852.58	+7.77
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.336	1.341	0
/error-in-render avg req/sec	1870.97	1864.08	⚠️ -6.89

Client Bundles (main, webpack, commons)

	vercel/next.js canary	shuding/next.js shu/9f14	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.6 kB	28.6 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.5 kB	72.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	shuding/next.js shu/9f14	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	shuding/next.js shu/9f14	Change
_app-HASH.js gzip	1.35 kB	1.35 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.37 kB	2.37 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	622 B	622 B	✓
image-HASH.js gzip	4.53 kB	4.53 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	1.9 kB	1.9 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	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	13.3 kB	13.3 kB	✓

Client Build Manifests

	vercel/next.js canary	shuding/next.js shu/9f14	Change
_buildManifest.js gzip	458 B	458 B	✓
Overall change	458 B	458 B	✓

Rendered Page Sizes

	vercel/next.js canary	shuding/next.js shu/9f14	Change
index.html gzip	530 B	530 B	✓
link.html gzip	545 B	545 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Commit: 948a4ec

move fs

269b90a

ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Dec 6, 2021

shuding changed the title ~~move fs~~ Refactor FS related methods in the Base Server Dec 6, 2021

shuding changed the title ~~Refactor FS related methods in the Base Server~~ Refactor FS references in the Base Server Dec 6, 2021

shuding marked this pull request as ready for review December 6, 2021 16:13

shuding requested review from huozhi, ijjk, styfle and timneutkens as code owners December 6, 2021 16:13

This comment has been minimized.

Sign in to view

karaggeorge reviewed Dec 6, 2021

View reviewed changes

karaggeorge previously approved these changes Dec 6, 2021

View reviewed changes

fix failing tests; remove unused imports

82ba899

shuding dismissed karaggeorge’s stale review via 82ba899 December 6, 2021 21:52

Merge branch 'canary' into shu/9f14

e24f6e3

huozhi reviewed Dec 6, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

huozhi approved these changes Dec 6, 2021

View reviewed changes

Merge branch 'canary' into shu/9f14

948a4ec

shuding merged commit db773c4 into vercel:canary Dec 7, 2021

shuding deleted the shu/9f14 branch December 7, 2021 01:14

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

Refactor FS references in the Base Server #32179

Refactor FS references in the Base Server #32179

shuding commented Dec 6, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

karaggeorge Dec 6, 2021

shuding Dec 6, 2021

karaggeorge left a comment

huozhi Dec 6, 2021

shuding Dec 6, 2021

This comment has been minimized.

ijjk commented Dec 6, 2021

Refactor FS references in the Base Server #32179

Refactor FS references in the Base Server #32179

Conversation

shuding commented Dec 6, 2021 • edited

Bug

Feature

Documentation / Examples

This comment has been minimized.

This comment has been minimized.

karaggeorge Dec 6, 2021

Choose a reason for hiding this comment

shuding Dec 6, 2021

Choose a reason for hiding this comment

karaggeorge left a comment

Choose a reason for hiding this comment

huozhi Dec 6, 2021

Choose a reason for hiding this comment

shuding Dec 6, 2021

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Dec 6, 2021

Stats from current PR

shuding commented Dec 6, 2021 •

edited