Create Base Server #32066

shuding · 2021-12-02T21:38:10Z

#31506: Similar to the approaches in #31971 and #32058, this PR creates a NextNodeServer class and will extend node-specific APIs on top of the base server class.

next-server stays unchanged, but it now extends the base-server with Node specific APIs. Note that this is just an initial PR which is not complete.

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

devknoll

Please change this to use composition rather than inheritance 🙏 🙏 🙏 We're already brushing up on the limits here: next-dev-server extends next-node-server, but what if we want to share functions between a node dev server and a different type?

What does getHasStaticDir even mean in a server without a filesystem? Etc

Instead, NextServer should just be an object that can be constructed with options, and we should use those options to tailor to the behavior. For example:

// The standalone Node.js Server might be constructed like this:
const server = new NextServer({
  routes: [
    ...routesFromStaticDir(join(dir, 'static')),
    ...routesFromPagesManifest(join(serverBuildDir, PAGES_MANIFEST)),
  ]
})

// The Vercel minimal-mode server might be constructed like this:
const server = new NextServer({
  // Note: static files are served by the CDN, not `NextServer`
  ...routesFromPagesManifest(join(serverBuildDir, PAGES_MANIFEST)),
})

With an approach like this, the base NextServer doesn't need to know what a PagesManifest is, what a MiddlewareManifest is, etc. In fact, once you take this to the extreme, NextServer actually just becomes a router...

devknoll · 2021-12-02T23:21:45Z

packages/next/server/next-server.ts

@@ -612,6 +607,22 @@ export default class Server {
    return this.getPrerenderManifest().preview
  }

+  protected getPagesManifest(): PagesManifest | undefined {


Shouldn't these go in the Node impl?

I think the idea is to move things progressively

Yeah this is just an initial PR and then we can move things in parallel.

packages/next/server/next-server.ts

devknoll · 2021-12-02T23:44:57Z

packages/next/server/next-server.ts

  protected generatePublicRoutes(): Route[] {
+    if (!fs.existsSync(this.publicDir)) return []


Doesn't this depend on the server (i.e. whether an fs is available)?

javivelasco · 2021-12-03T12:45:50Z

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

@@ -700,6 +700,13 @@ export default class DevServer extends Server {
    })
  }

+  protected getPagesManifest(): undefined {
+    return undefined


I'd maybe make it throw as it is totally unexpected to call this in the dev server.

I refactored the base class to make the constructor always call this method, because it doesn’t know if it’s extended by the dev server or not

javivelasco · 2021-12-03T12:47:22Z

packages/next/server/next-server.ts

@@ -239,7 +239,7 @@ export default class Server {

    this.distDir = join(this.dir, this.nextConfig.distDir)
    this.publicDir = join(this.dir, CLIENT_PUBLIC_FILES_PATH)
-    this.hasStaticDir = !minimalMode && fs.existsSync(join(this.dir, 'static'))
+    this.hasStaticDir = !minimalMode && this.getHasStaticDir()


Maybe instead of a method we can make this a property that must be initialized by the constructor of the implementing class.

Yes let's do that in follow-up PRs! Gonna ship this one first so it will unblock us.

javivelasco · 2021-12-03T12:48:03Z

packages/next/server/next-server.ts

@@ -612,6 +607,22 @@ export default class Server {
    return this.getPrerenderManifest().preview
  }

+  protected getPagesManifest(): PagesManifest | undefined {


I think the idea is to move things progressively

shuding · 2021-12-03T13:21:50Z

@devknoll

Instead, NextServer should just be an object that can be constructed with options, and we should use those options to tailor to the behavior

I think this will be a longer term goal that needs even more work, it also requires use to rewrite the places where we initiate a NextServer/NextDevServer so we can pass those manifests etc. to the constructor.

Also, one concern of this is bundle size since all the logic will be existing in the Server class as different branches, even if it’s not needed by the current runtime.

but what if we want to share functions between a node dev server and a different type

We can make those utils so they can be shared across runtimes.

What does getHasStaticDir even mean in a server without a filesystem? Etc

Those are the things that we can abstract out further, to make the methods of the base class minimal as possible, and runtime agnostic. In the getHasStaticDir case we can rename it to getHasStaticFallbacks For example, and extend it with fs impl in the node server and return false for other runtimes.

huozhi

~~Continue in the following PRs sounds good to me~~
Hold until we could launch this

ijjk · 2021-12-03T19:58:13Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	shuding/next.js shu/084c	Change
buildDuration	20.4s	20.7s	⚠️ +298ms
buildDurationCached	3.8s	3.8s	-7ms
nodeModulesSize	347 MB	347 MB	⚠️ +3.19 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	shuding/next.js shu/084c	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.317	3.369	⚠️ +0.05
/ avg req/sec	753.67	742.13	⚠️ -11.54
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.471	1.505	⚠️ +0.03
/error-in-render avg req/sec	1700.06	1661.32	⚠️ -38.74

Client Bundles (main, webpack, commons)

	vercel/next.js canary	shuding/next.js shu/084c	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/084c	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/084c	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/084c	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/084c	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 (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	shuding/next.js shu/084c	Change
buildDuration	22s	21.9s	-55ms
buildDurationCached	3.7s	3.8s	⚠️ +76ms
nodeModulesSize	347 MB	347 MB	⚠️ +3.19 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	shuding/next.js shu/084c	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.291	3.307	⚠️ +0.02
/ avg req/sec	759.73	755.97	⚠️ -3.76
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.547	1.503	-0.04
/error-in-render avg req/sec	1616.08	1663.29	+47.21

Client Bundles (main, webpack, commons)

	vercel/next.js canary	shuding/next.js shu/084c	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/084c	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/084c	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/084c	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/084c	Change
index.html gzip	532 B	532 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Commit: b9f02ac

shuding · 2021-12-04T00:04:29Z

Created #32105 instead.

Same as #32105 (closed due to conflicts): > Explained in #32066, rename next-server as base-server and extend NextNodeServer on top of it. > > Note that this is just an initial PR so no method is ported. ## 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`

shuding added 2 commits December 2, 2021 22:20

move fs usage into methods

dcd8496

create next-node-server

a809a17

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

fix type error

640d38b

This comment has been minimized.

Sign in to view

shuding marked this pull request as ready for review December 2, 2021 23:16

shuding requested review from huozhi, ijjk, padmaia, styfle and timneutkens as code owners December 2, 2021 23:16

shuding requested review from javivelasco, devknoll and karaggeorge December 2, 2021 23:16

devknoll reviewed Dec 3, 2021

View reviewed changes

javivelasco reviewed Dec 3, 2021

View reviewed changes

use abstract class and methods

720e895

This comment has been minimized.

Sign in to view

Merge branch 'canary' into shu/084c

5a72ff6

This comment has been minimized.

Sign in to view

shuding added 2 commits December 3, 2021 19:08

Merge branch 'canary' into shu/084c

7657722

Merge branch 'canary' into shu/084c

3dc8ce5

huozhi previously approved these changes Dec 3, 2021

View reviewed changes

shuding requested review from huozhi and removed request for karaggeorge December 3, 2021 18:31

rename file names

f213e90

shuding dismissed huozhi’s stale review via f213e90 December 3, 2021 18:41

This comment has been minimized.

Sign in to view

Merge branch 'canary' into shu/084c

b9f02ac

shuding closed this Dec 3, 2021

shuding deleted the shu/084c branch December 3, 2021 23:59

shuding mentioned this pull request Dec 4, 2021

Create Base Server #32105

Closed

10 tasks

shuding mentioned this pull request Dec 5, 2021

Create Base Server #32154

Merged

10 tasks

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

Create Base Server #32066

Create Base Server #32066

shuding commented Dec 2, 2021 •

edited

Loading

This comment has been minimized.

devknoll left a comment •

edited

Loading

devknoll Dec 2, 2021

javivelasco Dec 3, 2021

shuding Dec 3, 2021

devknoll Dec 2, 2021

javivelasco Dec 3, 2021

shuding Dec 3, 2021

javivelasco Dec 3, 2021

shuding Dec 3, 2021

javivelasco Dec 3, 2021

shuding commented Dec 3, 2021

This comment has been minimized.

This comment has been minimized.

huozhi left a comment •

edited

Loading

This comment has been minimized.

ijjk commented Dec 3, 2021

shuding commented Dec 4, 2021

		protected generatePublicRoutes(): Route[] {
		if (!fs.existsSync(this.publicDir)) return []

Create Base Server #32066

Create Base Server #32066

Conversation

shuding commented Dec 2, 2021 • edited Loading

Bug

Feature

Documentation / Examples

This comment has been minimized.

devknoll left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shuding commented Dec 3, 2021

This comment has been minimized.

This comment has been minimized.

huozhi left a comment • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Dec 3, 2021

Stats from current PR

shuding commented Dec 4, 2021

shuding commented Dec 2, 2021 •

edited

Loading

devknoll left a comment •

edited

Loading

huozhi left a comment •

edited

Loading