-
-
Notifications
You must be signed in to change notification settings - Fork 747
refactor: check neutral platform #12534
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 rspack 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 aims to add support for neutral platform in rspack, enabling prefetch and preload functionality for platforms that are neither explicitly web nor node. The main approach is to conditionally include runtime document checks based on whether the platform is "neutral" (i.e., cannot be determined at compile time).
Key Issues Identified
- Critical Bug: The variable
is_neutral_platformis used in multiple files but is never defined in any of the generate methods. - Critical Bug: The type
CompilerPlatformis imported but doesn't exist in rspack_core. - Critical Bug:
Compiler::new()is being called with an additionalplatformparameter that doesn't exist in its signature. - Moderate Bug: The css_loading_with_hmr.ejs template is not updated to use the conditional document check pattern.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| module_chunk_loading_with_preload.ejs | Adds conditional document check for neutral platforms in preload handler |
| module_chunk_loading_with_prefetch.ejs | Adds conditional document check for neutral platforms in prefetch handler |
| module_chunk_loading.rs | Passes is_neutral_platform to templates (but variable is undefined) |
| css/runtime/mod.rs | Passes is_neutral_platform to CSS templates (but variable is undefined) |
| css_loading_with_preload.ejs | Adds conditional document check for neutral platforms in CSS preload |
| css_loading_with_prefetch.ejs | Adds conditional document check for neutral platforms in CSS prefetch |
| defaults.rs | Updates test to handle tuple return from build method |
| builder/target.rs | Adds From implementation to convert TargetProperties to CompilerPlatform |
| builder/mod.rs | Updates build method to return tuple and pass platform to Compiler::new |
The PR is incomplete and missing several critical pieces:
- Definition of
CompilerPlatformtype - Calculation of
is_neutral_platformvariable - Updates to
Compilerstruct andnewmethod to accept platform - Update to css_loading_with_hmr.ejs template
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9ca59b4 to
5697ed6
Compare
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
❌ Size increased by 8.13KB from 47.87MB to 47.88MB (⬆️0.02%) |
CodSpeed Performance ReportMerging #12534 will not alter performanceComparing Summary
Footnotes
|
Summary
This PR adds support for neutral platform in rspack, enabling prefetch and preload functionality for platforms that are neither web nor node. Previously, prefetch and preload were only available when the environment supports document APIs. With this change, neutral platforms can also utilize these features.
The main changes include:
Checklist