Fix serverside asset modules #27413

Janpot · 2021-07-22T17:32:26Z

Reading local files with asset modules (as per this method) broke after #26256
The new value for publicPath seems like it was intended for client side only.
I also removed a comment that has lost its relevance after that PR

ijjk · 2021-07-22T18:44:04Z

Failing test suites

Commit: 170e77d

test/integration/server-asset-modules/test/index.test.js

serverside asset modules > dev mode > should enable reading local files in api routes
serverside asset modules > production mode > should enable reading local files in api routes
serverside asset modules > serverless mode > should enable reading local files in api routes

Expand output

● serverside asset modules › dev mode › should enable reading local files in api routes

thrown: "Exceeded timeout of 5000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  29 | describe('serverside asset modules', () => {
  30 |   describe('dev mode', () => {
> 31 |     beforeAll(async () => {
     |     ^
  32 |       appPort = await findPort()
  33 |       app = await launchApp(appDir, appPort)
  34 |     })

  at integration/server-asset-modules/test/index.test.js:31:5
  at integration/server-asset-modules/test/index.test.js:30:3
  at Object.<anonymous> (integration/server-asset-modules/test/index.test.js:29:1)

● serverside asset modules › production mode › should enable reading local files in api routes

thrown: "Exceeded timeout of 5000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  39 |
  40 |   describe('production mode', () => {
> 41 |     beforeAll(async () => {
     |     ^
  42 |       const curConfig = await fs.readFile(nextConfig, 'utf8')
  43 |
  44 |       if (curConfig.includes('target')) {

  at integration/server-asset-modules/test/index.test.js:41:5
  at integration/server-asset-modules/test/index.test.js:40:3
  at Object.<anonymous> (integration/server-asset-modules/test/index.test.js:29:1)

● serverside asset modules › serverless mode › should enable reading local files in api routes

thrown: "Exceeded timeout of 5000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  58 |     let origNextConfig
  59 |
> 60 |     beforeAll(async () => {
     |     ^
  61 |       origNextConfig = await fs.readFile(nextConfig, 'utf8')
  62 |       await fs.writeFile(
  63 |         nextConfig,

  at integration/server-asset-modules/test/index.test.js:60:5
  at integration/server-asset-modules/test/index.test.js:57:3
  at Object.<anonymous> (integration/server-asset-modules/test/index.test.js:29:1)

● Test suite failed to run

TypeError: Cannot read property 'pid' of undefined

  297 | export async function killApp(instance) {
  298 |   await new Promise((resolve, reject) => {
> 299 |     treeKill(instance.pid, (err) => {
      |                       ^
  300 |       if (err) {
  301 |         if (
  302 |           process.platform === 'win32' &&

  at lib/next-test-utils.js:299:23
  at killApp (lib/next-test-utils.js:298:9)
  at integration/server-asset-modules/test/index.test.js:35:20

● Test suite failed to run

TypeError: Cannot read property 'pid' of undefined

  297 | export async function killApp(instance) {
  298 |   await new Promise((resolve, reject) => {
> 299 |     treeKill(instance.pid, (err) => {
      |                       ^
  300 |       if (err) {
  301 |         if (
  302 |           process.platform === 'win32' &&

  at lib/next-test-utils.js:299:23
  at killApp (lib/next-test-utils.js:298:9)
  at integration/server-asset-modules/test/index.test.js:52:20

● Test suite failed to run

TypeError: Cannot read property 'pid' of undefined

  297 | export async function killApp(instance) {
  298 |   await new Promise((resolve, reject) => {
> 299 |     treeKill(instance.pid, (err) => {
      |                       ^
  300 |       if (err) {
  301 |         if (
  302 |           process.platform === 'win32' &&

  at lib/next-test-utils.js:299:23
  at killApp (lib/next-test-utils.js:298:9)
  at integration/server-asset-modules/test/index.test.js:74:13

ijjk · 2021-09-09T07:51:15Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall decrease ✓

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
buildDuration	13.9s	13.9s	-48ms
buildDurationCached	3.4s	3.6s	⚠️ +193ms
nodeModulesSize	48.2 MB	48.2 MB	-260 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.645	2.64	0
/ avg req/sec	945.27	946.84	+1.57
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.449	1.436	-0.01
/error-in-render avg req/sec	1724.92	1740.62	+15.7

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
745.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	23.3 kB	23.3 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	67.2 kB	67.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
_app-HASH.js gzip	979 B	979 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.14 kB	4.14 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	318 B	318 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	13 kB	13 kB	✓

Client Build Manifests

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
_buildManifest.js gzip	492 B	492 B	✓
Overall change	492 B	492 B	✓

Rendered Page Sizes

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
index.html gzip	539 B	539 B	✓
link.html gzip	552 B	552 B	✓
withRouter.html gzip	533 B	533 B	✓
Overall change	1.62 kB	1.62 kB	✓

Webpack 4 Mode (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
buildDuration	11.5s	11.3s	-153ms
buildDurationCached	4.7s	4.7s	-10ms
nodeModulesSize	48.2 MB	48.2 MB	-260 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.73	2.758	⚠️ +0.03
/ avg req/sec	915.73	906.48	⚠️ -9.25
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.48	1.49	⚠️ +0.01
/error-in-render avg req/sec	1689.75	1677.86	⚠️ -11.89

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
16.HASH.js gzip	186 B	186 B	✓
677f882d2ed8..HASH.js gzip	14.1 kB	14.1 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	10.7 kB	10.7 kB	✓
webpack-HASH.js gzip	1.19 kB	1.19 kB	✓
Overall change	68.1 kB	68.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
_app-HASH.js gzip	964 B	964 B	✓
_error-HASH.js gzip	3.8 kB	3.8 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.87 kB	2.87 kB	✓
head-HASH.js gzip	3.06 kB	3.06 kB	✓
hooks-HASH.js gzip	924 B	924 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.03 kB	3.03 kB	✓
withRouter-HASH.js gzip	295 B	295 B	✓
30809af5c834..565.css gzip	125 B	125 B	✓
Overall change	18.1 kB	18.1 kB	✓

Client Build Manifests

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	Janpot/next.js server-asset-modules	Change
index.html gzip	585 B	585 B	✓
link.html gzip	597 B	597 B	✓
withRouter.html gzip	578 B	578 B	✓
Overall change	1.76 kB	1.76 kB	✓

Commit: e59fa7e

timneutkens · 2021-09-09T10:38:24Z

#28940 should fix this 👍

Currently `new URL()` for server assets is completely broken because of the `publicPath` that is used for them too. `new URL()` for SSR is broken on windows as it's using absolute urls on the windows filesystem. And `new URL()` is using an incorrect filename * Place all `asset`s correctly in `/_next/static/media` with `[name].[hash:8][ext]` * Added a separate runtime chunk for api entries, without `publicPath` * Introduce separate layer for api entries, which uses server-side URLs. * Otherwise new URL() will return a faked relative URL, that is identical in SSR and CSR * Disables react-refresh for api entries Fixes #27413 ## Bug - [ ] Related issues linked using `fixes #number` - [x] 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` - [x] 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

Currently `new URL()` for server assets is completely broken because of the `publicPath` that is used for them too. `new URL()` for SSR is broken on windows as it's using absolute urls on the windows filesystem. And `new URL()` is using an incorrect filename * Place all `asset`s correctly in `/_next/static/media` with `[name].[hash:8][ext]` * Added a separate runtime chunk for api entries, without `publicPath` * Introduce separate layer for api entries, which uses server-side URLs. * Otherwise new URL() will return a faked relative URL, that is identical in SSR and CSR * Disables react-refresh for api entries Fixes vercel#27413 ## Bug - [ ] Related issues linked using `fixes #number` - [x] 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` - [x] 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

reset publicPath to empty string on server

c9b72da

ijjk added the type: next label Jul 22, 2021

This comment has been minimized.

Sign in to view

Janpot added 3 commits July 22, 2021 19:52

add test

0d47f37

didn't intend to push this

7c823c9

Merge branch 'canary' into server-asset-modules

170e77d

This comment has been minimized.

Sign in to view

Janpot added 2 commits July 22, 2021 20:47

add test timeout

4c9db62

Merge branch 'canary' into server-asset-modules

36e8101

This comment has been minimized.

Sign in to view

Janpot marked this pull request as ready for review July 22, 2021 21:04

Janpot requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners July 22, 2021 21:04

This was referenced Jul 24, 2021

Fix generator publicPath behavior when empty string webpack/webpack#13858

Merged

Next.js API routes (and pages) should support reading files #8251

Closed

Merge branch 'canary' into server-asset-modules

2cbde8d

This comment has been minimized.

Sign in to view

Merge branch 'canary' into server-asset-modules

e59fa7e

timneutkens mentioned this pull request Sep 9, 2021

add support for new URL() #28940

Merged

10 tasks

Janpot closed this Sep 9, 2021

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

Fix serverside asset modules #27413

Fix serverside asset modules #27413

Janpot commented Jul 22, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 22, 2021

This comment has been minimized.

This comment has been minimized.

ijjk commented Sep 9, 2021

timneutkens commented Sep 9, 2021

Fix serverside asset modules #27413

Fix serverside asset modules #27413

Conversation

Janpot commented Jul 22, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

ijjk commented Jul 22, 2021

Failing test suites

This comment has been minimized.

This comment has been minimized.

ijjk commented Sep 9, 2021

Stats from current PR

timneutkens commented Sep 9, 2021

Janpot commented Jul 22, 2021 •

edited

Loading