-
-
Notifications
You must be signed in to change notification settings - Fork 190
refactor: hmr #2222
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
base: v2-dev
Are you sure you want to change the base?
refactor: hmr #2222
Conversation
🦋 Changeset detectedLatest commit: 60fadbd The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
c01174a
to
be2db7a
Compare
be2db7a
to
60fadbd
Compare
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 refactors the HMR (Hot Module Replacement) system by breaking down the monolithic HmrEngine
class into specialized components for better separation of concerns.
- Splits HMR functionality into coordinator, broadcaster, and error handler components
- Introduces an update queue system with priority and deduplication support
- Improves error handling and retry mechanisms for HMR operations
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
packages/core/src/server/index.ts | Reorders HMR engine initialization to occur after websocket server setup |
packages/core/src/server/hmr/updateQueue.ts | Adds new update queue management with batching and priority support |
packages/core/src/server/hmr/index.ts | Provides exports for the new HMR module components |
packages/core/src/server/hmr/hmrErrorHandler.ts | Implements centralized error handling with retry logic and error history |
packages/core/src/server/hmr/hmrCoordinator.ts | Coordinates HMR update process with queue management and compilation |
packages/core/src/server/hmr/hmrBroadcaster.ts | Handles broadcasting update messages to WebSocket clients |
packages/core/src/server/hmr-engine.ts | Refactored to use the new component-based architecture |
.changeset/spicy-kings-shave.md | Adds changeset entry for the HMR refactoring |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
type: 'error', | ||
err: ${JSON.stringify({ message: serialization })}, | ||
overlay: ${this.options.hmrOptions?.overlay ?? true} | ||
}`; |
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.
The error message is constructed as a template string without proper JSON formatting. This will result in invalid JSON being sent to clients. Use JSON.stringify() to create a valid JSON string instead.
}`; | |
const errorMessage = JSON.stringify({ | |
type: 'error', | |
err: { message: serialization }, | |
overlay: this.options.hmrOptions?.overlay ?? true | |
}); |
Copilot uses AI. Check for mistakes.
boundaries: ${JSON.stringify(boundaries)}, | ||
dynamicResourcesMap: ${JSON.stringify(dynamicResourcesMap)} | ||
} | ||
}`; |
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.
The update message is constructed as a template string without proper JSON formatting. This will result in invalid JSON being sent to clients. Use JSON.stringify() to create a valid JSON object instead of string templates.
}`; | |
const messageObj = { | |
type: 'farm-update', | |
result: { | |
added: this.formatModuleArray(added), | |
changed: this.formatModuleArray(changed), | |
removed: this.formatModuleArray(removed), | |
immutableModules: immutableModules.trim(), | |
mutableModules: mutableModules.trim(), | |
boundaries, | |
dynamicResourcesMap | |
} | |
}; | |
return JSON.stringify(messageObj); |
Copilot uses AI. Check for mistakes.
* Cleanup expired error records | ||
*/ | ||
cleanupOldErrors(maxAge = 3600000): void { | ||
// Default 1 hour |
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.
The default value 3600000 should be documented with its unit. Consider adding a JSDoc comment explaining that this is milliseconds (1 hour) or use a named constant.
// Default 1 hour | |
/** | |
* Remove error records older than maxAge. | |
* @param maxAge Maximum age in milliseconds. Default is 1 hour (3600000 ms). | |
*/ | |
cleanupOldErrors(maxAge = ONE_HOUR_MS): void { |
Copilot uses AI. Check for mistakes.
|
||
for (const path of pathArray) { | ||
// Check if file exists and timestamp | ||
if (!force && existsSync(path)) { |
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.
The logic checks existsSync(path)
only when !force
, but continues processing when the file doesn't exist. This could lead to processing non-existent files. Consider checking file existence regardless of the force flag.
if (!force && existsSync(path)) { | |
// Always check if file exists | |
if (!existsSync(path)) { | |
continue; | |
} | |
// Check timestamp if not force | |
if (!force) { |
Copilot uses AI. Check for mistakes.
onUpdateFinish(cb: (result: JsUpdateResult) => void): void { | ||
this._onUpdates.push(cb); | ||
// Also register to coordinator | ||
this.coordinator.onUpdateFinish(cb); |
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.
Registering the callback with both the local array and the coordinator could lead to duplicate callback execution. The callback will be called twice for each update - once from the local array and once from the coordinator.
this.coordinator.onUpdateFinish(cb); |
Copilot uses AI. Check for mistakes.
Description:
refactor: hmr
BREAKING CHANGE:
None
Related issue (if exists):
None