-
-
Notifications
You must be signed in to change notification settings - Fork 232
refactor(assets-middleware): simplify context management #6269
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
Conversation
✅ Deploy Preview for rsbuild ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR simplifies context management in the assets middleware by using Rsbuild's internal context for build status tracking instead of maintaining separate ready state.
- Removes the experimental
locals.webpackobject and setupHooks file - Consolidates ready callback logic directly into the middleware
- Passes the full context instead of just environments to the middleware
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/server/buildManager.ts | Updates assetsMiddleware call to pass full context instead of environments |
| packages/core/src/server/assets-middleware/setupHooks.ts | Removes entire file that managed ready state and callbacks |
| packages/core/src/server/assets-middleware/middleware.ts | Simplifies middleware by removing locals.webpack and using passed ready function |
| packages/core/src/server/assets-middleware/index.ts | Consolidates ready callback logic and uses context.buildState for status checking |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| const ready = (callback: () => void) => { | ||
| if (context.buildState.status === 'done') { | ||
| callback(); | ||
| } else { | ||
| callbacks.push(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Block dev requests when using a custom compiler
The middleware now waits for context.buildState.status === 'done' before invoking queued callbacks. buildState is only updated inside Rsbuild’s own createCompiler, but startDevServer still allows a caller-provided customCompiler (devServer.ts lines 182‑184) that never touches context.buildState. With a custom compiler the status remains idle, so the ready queue is never drained and every asset request hangs indefinitely. This is a regression from the previous hook-based Context which worked for arbitrary compilers. Consider falling back to compiler hooks when buildState isn’t driven by the given compiler or updating buildState when a custom compiler is supplied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think customCompiler doesn't work properly in the previous implementation and needs to be deprecated

Summary
Simplify context management in the assets middleware:
locals.webpackobject, it's a experimental API of webpack-dev-middleware and not considered a public API for Rsbuild.Related links
https://github.com/webpack/webpack-dev-middleware?tab=readme-ov-file#server-side-rendering
Checklist