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

Fix issue with outputFileTracingExcludes and pages/api edge runtime #59157

Conversation

timneutkens
Copy link
Member

What?

Ensures Object.entries is not called on the Map. Seems this only fails in a very particular case but potentially this fixes other issues than the one I added in the tests too.

How?

Object.entries() results in an empty array when called on a Map. Created a shared type declaration for the value and removed the Object.entries. Benefit of this is that we can skip the loop as well.

@timneutkens
Copy link
Member Author

timneutkens commented Dec 1, 2023

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@ijjk
Copy link
Member

ijjk commented Dec 1, 2023

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
buildDuration 12.7s 12.6s N/A
buildDurationCached 7.1s 6s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +4.11 kB
nextStartRea..uration (ms) 431ms 422ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
170-HASH.js gzip 26.8 kB 26.8 kB N/A
199.HASH.js gzip 181 B 185 B N/A
3f784ff6-HASH.js gzip 53.3 kB 53.3 kB
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 240 B 241 B N/A
main-HASH.js gzip 31.7 kB 31.7 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB N/A
Overall change 98.5 kB 98.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
_app-HASH.js gzip 195 B 194 B N/A
_error-HASH.js gzip 183 B 182 B N/A
amp-HASH.js gzip 501 B 501 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 255 B 255 B
head-HASH.js gzip 349 B 350 B N/A
hooks-HASH.js gzip 368 B 369 B N/A
image-HASH.js gzip 4.27 kB 4.27 kB N/A
index-HASH.js gzip 255 B 256 B N/A
link-HASH.js gzip 2.61 kB 2.6 kB N/A
routerDirect..HASH.js gzip 311 B 309 B N/A
script-HASH.js gzip 384 B 384 B
withRouter-HASH.js gzip 307 B 306 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 1.57 kB 1.57 kB
Client Build Manifests
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
_buildManifest.js gzip 484 B 482 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
index.html gzip 528 B 529 B N/A
link.html gzip 542 B 541 B N/A
withRouter.html gzip 524 B 524 B
Overall change 524 B 524 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
edge-ssr.js gzip 93.7 kB 93.7 kB N/A
page.js gzip 146 kB 146 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
middleware-b..fest.js gzip 628 B 622 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 37.4 kB 37.4 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 2.07 kB 2.07 kB
Next Runtimes
vercel/next.js canary vercel/next.js 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime Change
app-page-exp...dev.js gzip 168 kB 168 kB
app-page-exp..prod.js gzip 94.1 kB 94.1 kB
app-page-tur..prod.js gzip 94.8 kB 94.8 kB
app-page-tur..prod.js gzip 89.4 kB 89.4 kB
app-page.run...dev.js gzip 138 kB 138 kB
app-page.run..prod.js gzip 88.7 kB 88.7 kB
app-route-ex...dev.js gzip 24 kB 24 kB
app-route-ex..prod.js gzip 16.6 kB 16.6 kB
app-route-tu..prod.js gzip 16.6 kB 16.6 kB
app-route-tu..prod.js gzip 16.2 kB 16.2 kB
app-route.ru...dev.js gzip 23.4 kB 23.4 kB
app-route.ru..prod.js gzip 16.2 kB 16.2 kB
pages-api-tu..prod.js gzip 9.37 kB 9.37 kB
pages-api.ru...dev.js gzip 9.64 kB 9.64 kB
pages-api.ru..prod.js gzip 9.37 kB 9.37 kB
pages-turbo...prod.js gzip 21.9 kB 21.9 kB
pages.runtim...dev.js gzip 22.5 kB 22.5 kB
pages.runtim..prod.js gzip 21.9 kB 21.9 kB
server.runti..prod.js gzip 49.4 kB 49.4 kB
Overall change 930 kB 930 kB
Commit: 72286e9

packages/next/src/build/index.ts Outdated Show resolved Hide resolved
@@ -1108,7 +1108,8 @@ export default async function build(
config,
distDir,
pageKeys,
pageInfos: [],
// TODO: Is this wrong?
pageInfos: new Map(),
Copy link
Member

Choose a reason for hiding this comment

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

This can't be a Map when using Worker as it's not serializable by default.

Copy link
Member

Choose a reason for hiding this comment

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

We could omit this here and instead have a default arg value in the function since we aren't passing actual page info through here.

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.

Note above about this breaking the worker handling for build traces.

Copy link
Contributor

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

Approving because this is landable however I do recommend we make the additional change now or in a later pr

@@ -623,6 +628,9 @@ export async function collectBuildTraces({
}

const { entryNameFilesMap } = buildTraceContext?.chunksTrace || {}
const infos =
pageInfos instanceof Map ? pageInfos : deserializePageInfos(pageInfos)

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that this is potentially a worker boundary means the interface should be as small as possible. PageInfos is only used for one check so it’d be even better if we only passed the info needed. I’d just turn this into an array of edge runtime routes. We can construct this parallel data structure when we build page infos. The. I’ve thing is that if you aren’t using edge at all then we pay zero when crossing the worker boundary and when you do we only send the bare minimum info

@timneutkens timneutkens merged commit a0c68c5 into canary Dec 15, 2023
64 of 69 checks passed
@timneutkens timneutkens deleted the 12-01-Fix_issue_with_outputFileTracingExcludes_and_pages/api_edge_runtime branch December 15, 2023 15:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2023
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.

4 participants