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

Implement foundations for additional bundler option #75981

Open
wants to merge 1 commit into
base: wbinnssmith/get-rspack-core
Choose a base branch
from

Conversation

wbinnssmith
Copy link
Member

@wbinnssmith wbinnssmith commented Feb 12, 2025

This implements core code from https://github.com/vercel/next.js/tree/wbinnssmith/try-ci-test for rspack. It does not include code for the built-in flight or app loader.

These code paths are currently taken when NEXT_RSPACK is set when running Next.js. Ultimately, this will be set with a new Next.js plugin, which will also install the necessary dependencies.

How?

  • Adds new npm scripts for running tests with Rspack enabled
  • Adjusts build trace collection to handle Rspack's chunk output
  • Updates compiler configuration to support Rspack's devtool settings
  • Modifies CSS extraction plugin usage to work with Rspack's implementation
  • Adds OpenTelemetry tracing support for Rspack builds

cc @hardfist

Copy link
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@wbinnssmith wbinnssmith requested review from bgw and ijjk February 12, 2025 19:51
@wbinnssmith wbinnssmith marked this pull request as ready for review February 12, 2025 19:51
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/get-rspack-core branch from a0248b5 to b8db5b9 Compare February 12, 2025 20:05
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/rspack-foundations branch from 8cf9f5a to 67c19ea Compare February 12, 2025 20:05
@@ -50,6 +51,10 @@ export function runCompiler(
inputFileSystem?: webpack.Compiler['inputFileSystem'],
]
> {
if (process.env.NEXT_RSPACK_OTEL) {

Choose a reason for hiding this comment

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

I think we don't need to integrate RSPACK_OTEL support, it can be removed

@@ -34,6 +34,8 @@ export const base = curry(function base(
if (ctx.isDevelopment) {
if (process.env.__NEXT_TEST_MODE && !process.env.__NEXT_TEST_WITH_DEVTOOL) {
config.devtool = false
} else if (process.env.NEXT_RSPACK) {
config.devtool = 'source-map'

Choose a reason for hiding this comment

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

why change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rspack currently still generates sourcemaps with file:// locations when using the eval devtool -- our current code to reverse maps expects webpack[-internal]://.

Similarly, Turbopack always uses separate map files and file:// locations. Is there a significant performance impact doing so with Rspack?

Copy link

@hardfist hardfist Feb 12, 2025

Choose a reason for hiding this comment

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

it seems a rspack's bug, i will fix it
Honestly I don't know why webpack choose eval for development by default(performance reason maybe), personally I like debug bundled code without sourcemap, so eval is always a trouble when sourcemap disabled

Copy link
Member Author

@wbinnssmith wbinnssmith Feb 12, 2025

Choose a reason for hiding this comment

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

Yeah, as far as I know it's for performance reasons. I'm guessing @sokra or @timneutkens could elaborate.

In my experience the separate map file and file uri sources make for a much better debugging experience. That and eval is constrained with things like content security policy, etc.

Choose a reason for hiding this comment

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

yeah I met eval csp problems in formbricks repo

Copy link

@hardfist hardfist Feb 12, 2025

Choose a reason for hiding this comment

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

image
that's weird, it seems both rspack and webpack generates same result when devtool set to 'eval'

Choose a reason for hiding this comment

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

ok i know the reason, the yj-try-ci branch doesn't enable EvalSourceMapDevToolPlugin for rspack, so moduleFilenameTemplate not working, I will fix it

Choose a reason for hiding this comment

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

ok, it's actually a bug of eval-source-map and fixed in web-infra-dev/rspack#9275

@@ -576,6 +578,8 @@ export const css = curry(async function css(
// Exclude extensions that webpack handles by default
exclude: [
/\.(js|mjs|jsx|ts|tsx)$/,
// TODO investigate why this is needed

Choose a reason for hiding this comment

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

this is used to workaround a old bug which maybe solved now , I need to check it

Choose a reason for hiding this comment

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

I checked using pages-dir and app-dir cases and it seems runs well without this hack, so it seems you can remove the hack now

@wbinnssmith wbinnssmith force-pushed the wbinnssmith/rspack-foundations branch 2 times, most recently from 5b9357e to 230496d Compare February 12, 2025 22:40
@wbinnssmith wbinnssmith force-pushed the wbinnssmith/get-rspack-core branch from b8db5b9 to 46fe50d Compare February 12, 2025 22:40
Comment on lines +3 to +7
if (process.env.NEXT_RSPACK) {
// silent rspack's schema check
process.env.RSPACK_CONFIG_VALIDATE = 'loose-silent'
}

Copy link
Member

Choose a reason for hiding this comment

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

ESM imports are hoisted, so this will get evaluated after the imports. Let's just move it after the imports so that ordering is more obvious.

// we aren't getting all chunks in the trace-entrypoint plugin
// with rspack currently so for now just add them manually for
// all trace files
if (process.env.NEXT_RSPACK) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we're checking this in multiple places in this file, let's hoist this into a constant at the top?

"test-start-turbo": "cross-env NEXT_TEST_MODE=start TURBOPACK=1 TURBOPACK_BUILD=1 pnpm testheadless",
"test-deploy": "cross-env NEXT_TEST_MODE=deploy pnpm testheadless",
"testonly-dev": "cross-env NEXT_TEST_MODE=dev pnpm testonly",
"testonly-dev": "cross-env NEXT_TEST_MODE=dev NEXT_RSPACK=1 pnpm testonly",
Copy link
Member

Choose a reason for hiding this comment

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

This should be in a separate rspack command.

@wbinnssmith wbinnssmith force-pushed the wbinnssmith/rspack-foundations branch from 230496d to b5f091e Compare February 13, 2025 02:48
@ijjk
Copy link
Member

ijjk commented Feb 13, 2025

Failing test suites

Commit: 690b8f6

pnpm test test/integration/error-plugin-stack-overflow/test/index.test.js

  • Reports stack trace when webpack plugin stack overflows > shows details in next build
Expand output

● Reports stack trace when webpack plugin stack overflows › shows details in next build

expect(received).toContain(expected) // indexOf

Expected substring: "caused by plugins in Compilation.hooks.processAssets"
Received string:    " ⚠ Linting is disabled.
unhandledRejection TypeError: Cannot read properties of undefined (reading 'toJson')

  at generateStats (../dist/build/compiler.js:13:39)
  at <unknown> (../dist/build/compiler.js:42:106)
  at Span.traceFn (../dist/trace/trace.js:150:20)
  at <unknown> (../dist/build/compiler.js:42:94)
  at finalCallback (../dist/compiled/webpack/bundle5.js:28:139789)
  at onCompiled (../dist/compiled/webpack/bundle5.js:28:139900)
  at <unknown> (../dist/compiled/webpack/bundle5.js:28:149435)
  at finalCallback (../dist/compiled/webpack/bundle5.js:28:100224)
  at <unknown> (../dist/compiled/webpack/bundle5.js:28:106390)
  at eval (../dist/compiled/webpack/bundle5.js:13:9218)
  "
  at Object.toContain (integration/error-plugin-stack-overflow/test/index.test.js:18:22)

Read more about building and testing Next.js in contributing.md.

pnpm test-dev-turbo test/development/acceptance/server-component-compiler-errors-in-pages.test.ts (turbopack)

  • Error Overlay for server components compiler errors in pages > importing 'next/cache' APIs in pages > unstable_cacheTag is not allowed
  • Error Overlay for server components compiler errors in pages > importing 'next/cache' APIs in pages > unstable_expirePath is not allowed
  • Error Overlay for server components compiler errors in pages > importing 'next/cache' APIs in pages > unstable_expireTag is not allowed
Expand output

● Error Overlay for server components compiler errors in pages › importing 'next/cache' APIs in pages › unstable_cacheTag is not allowed

Expected Redbox but found none

  225 |       )
  226 |
> 227 |       await session.assertHasRedbox()
      |       ^
  228 |       await expect(session.getRedboxSource()).resolves.toMatch(
  229 |         `You're importing a component that needs "${api}". That only works in a Server Component which is not supported in the pages/ directory.`
  230 |       )

  at development/acceptance/server-component-compiler-errors-in-pages.test.ts:227:7

● Error Overlay for server components compiler errors in pages › importing 'next/cache' APIs in pages › unstable_expirePath is not allowed

Expected Redbox but found none

  225 |       )
  226 |
> 227 |       await session.assertHasRedbox()
      |       ^
  228 |       await expect(session.getRedboxSource()).resolves.toMatch(
  229 |         `You're importing a component that needs "${api}". That only works in a Server Component which is not supported in the pages/ directory.`
  230 |       )

  at development/acceptance/server-component-compiler-errors-in-pages.test.ts:227:7

● Error Overlay for server components compiler errors in pages › importing 'next/cache' APIs in pages › unstable_expireTag is not allowed

Expected Redbox but found none

  225 |       )
  226 |
> 227 |       await session.assertHasRedbox()
      |       ^
  228 |       await expect(session.getRedboxSource()).resolves.toMatch(
  229 |         `You're importing a component that needs "${api}". That only works in a Server Component which is not supported in the pages/ directory.`
  230 |       )

  at development/acceptance/server-component-compiler-errors-in-pages.test.ts:227:7

Read more about building and testing Next.js in contributing.md.

@ijjk
Copy link
Member

ijjk commented Feb 13, 2025

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
buildDuration 20.9s 18.2s N/A
buildDurationCached 17.2s 14.6s N/A
nodeModulesSize 393 MB 393 MB ⚠️ +20.7 kB
nextStartRea..uration (ms) 464ms 464ms
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
5306-HASH.js gzip 55.1 kB 55.1 kB N/A
7048.HASH.js gzip 168 B 168 B
8377-HASH.js gzip 5.46 kB 5.46 kB N/A
bccd1874-HASH.js gzip 53 kB 53 kB N/A
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 245 B 245 B
main-HASH.js gzip 34.7 kB 34.7 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB
Overall change 2.12 kB 2.12 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
_app-HASH.js gzip 194 B 194 B
_error-HASH.js gzip 193 B 192 B N/A
amp-HASH.js gzip 513 B 511 B N/A
css-HASH.js gzip 342 B 342 B
dynamic-HASH.js gzip 1.84 kB 1.84 kB N/A
edge-ssr-HASH.js gzip 265 B 264 B N/A
head-HASH.js gzip 363 B 360 B N/A
hooks-HASH.js gzip 393 B 390 B N/A
image-HASH.js gzip 4.59 kB 4.59 kB N/A
index-HASH.js gzip 268 B 266 B N/A
link-HASH.js gzip 2.35 kB 2.35 kB
routerDirect..HASH.js gzip 327 B 326 B N/A
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 325 B 325 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.72 kB 3.72 kB
Client Build Manifests
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
_buildManifest.js gzip 749 B 747 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
index.html gzip 522 B 521 B N/A
link.html gzip 539 B 534 B N/A
withRouter.html gzip 519 B 516 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
edge-ssr.js gzip 130 kB 130 kB N/A
page.js gzip 211 kB 211 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
middleware-b..fest.js gzip 677 B 672 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.4 kB 31.4 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
app-page-exp...dev.js gzip 394 kB 394 kB
app-page-exp..prod.js gzip 133 kB 133 kB
app-page-tur..prod.js gzip 145 kB 145 kB
app-page-tur..prod.js gzip 141 kB 141 kB
app-page.run...dev.js gzip 382 kB 382 kB
app-page.run..prod.js gzip 129 kB 129 kB
app-route-ex...dev.js gzip 39.4 kB 39.4 kB
app-route-ex..prod.js gzip 25.7 kB 25.7 kB
app-route-tu..prod.js gzip 25.7 kB 25.7 kB
app-route-tu..prod.js gzip 25.5 kB 25.5 kB
app-route.ru...dev.js gzip 41 kB 41 kB
app-route.ru..prod.js gzip 25.5 kB 25.5 kB
dist_client_...dev.js gzip 356 B 356 B
dist_client_...dev.js gzip 349 B 349 B
pages-api-tu..prod.js gzip 9.72 kB 9.72 kB
pages-api.ru...dev.js gzip 11.8 kB 11.8 kB
pages-api.ru..prod.js gzip 9.72 kB 9.72 kB
pages-turbo...prod.js gzip 22 kB 22 kB
pages.runtim...dev.js gzip 31.6 kB 31.6 kB
pages.runtim..prod.js gzip 22 kB 22 kB
server.runti..prod.js gzip 61.2 kB 61.2 kB
Overall change 1.68 MB 1.68 MB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js wbinnssmith/rspack-foundations Change
0.pack gzip 2.12 MB 2.12 MB ⚠️ +405 B
index.pack gzip 75.5 kB 76.2 kB ⚠️ +701 B
Overall change 2.19 MB 2.19 MB ⚠️ +1.11 kB
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: 690b8f6

This implements core code from https://github.com/vercel/next.js/tree/wbinnssmith/try-ci-test for rspack. It does not include code for the built-in flight or app loader.

These code paths are currently taken when `NEXT_RSPACK` is set when running Next.js. Ultimately, this will be set with a new Next.js plugin, which will also install the necessary dependencies.

Co-authored-by: hardfist <[email protected]>
Co-authored-by: JJ Kasper <[email protected]>
Co-authored-by: Tim Neutkens <[email protected]>
const compilerResult = runWebpackSpan
.traceChild('webpack-generate-error-stats')
.traceFn(() =>
generateStats({ errors: [], warnings: [], stats }, stats!)
Copy link

@hardfist hardfist Feb 13, 2025

Choose a reason for hiding this comment

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

why change this file, it seems these changes is just reorder the code, I check the origin code and it seems works find in rspack


if (curResult.buildTraceContext?.chunksTrace) {
const { entryNameFilesMap } = curResult.buildTraceContext.chunksTrace!

Choose a reason for hiding this comment

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

seems just format change?

@@ -463,6 +471,19 @@ export async function collectBuildTraces({
curTracedFiles.add(file)
}

if (process.env.NEXT_RSPACK) {
for (const file of chunks) {

Choose a reason for hiding this comment

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

not sure whether these changes needed since it seems TraceEntryPointsPlugin runs on Rspack

@@ -35,7 +36,7 @@ function closeCompiler(compiler: webpack.Compiler | webpack.MultiCompiler) {
})
}

export function runCompiler(
export async function runCompiler(

Choose a reason for hiding this comment

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

no need after remove rspack oetl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants