-
Notifications
You must be signed in to change notification settings - Fork 5k
bake: keep a route failed when the "use client" file it imports fails to bundle #37850
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
Changes from 1 commit
8b4bda2
7854006
67cdd4f
c27f948
9775e59
6e24d33
e4a61a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2251,6 +2251,10 @@ fn check_route_failures( | |||||||||||||||||||||||||
| resp: DevResponse, | ||||||||||||||||||||||||||
| ) -> crate::Result<CheckResult> { | ||||||||||||||||||||||||||
| let mut gts = dev.init_graph_trace_state(0)?; | ||||||||||||||||||||||||||
| // The trace below collects into `failures_added`, which still holds the | ||||||||||||||||||||||||||
| // failures of the last bundle (it is only reset when the next one starts). | ||||||||||||||||||||||||||
| // Those may belong to files this route never imports. | ||||||||||||||||||||||||||
|
robobun marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| dev.incremental_result.failures_added.clear(); | ||||||||||||||||||||||||||
|
Comment on lines
+2254
to
+2255
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Clear failure state before graph-state allocation. Line 2253 can return before Line 2255 clears stale entries or registers deferred cleanup. A later route can then report failures from another route after this allocation error. Proposed fix- let mut gts = dev.init_graph_trace_state(0)?;
// Still holds the last bundle's failures, which this route may not import.
dev.incremental_result.failures_added.clear();
// Note: erase to a raw pointer so the deferred cleanup only fires on
// scope exit when no other borrow of `dev` is live.
let dev_ptr = std::ptr::from_mut::<DevServer>(dev);
scopeguard::defer! {
// SAFETY: see Note above.
unsafe { (*dev_ptr).incremental_result.failures_added.clear() }
};
+ let mut gts = dev.init_graph_trace_state(0)?;As per coding guidelines: “Every error, abort, and timeout path must complete the operation.” 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||||||
| // Note: erase to a raw pointer so the deferred cleanup only fires on | ||||||||||||||||||||||||||
| // scope exit when no other borrow of `dev` is live. | ||||||||||||||||||||||||||
| let dev_ptr = std::ptr::from_mut::<DevServer>(dev); | ||||||||||||||||||||||||||
|
|
@@ -4388,13 +4392,19 @@ pub(super) fn finalize_bundle( | |||||||||||||||||||||||||
| dev.incremental_result.html_routes_soft_affected.clear(); | ||||||||||||||||||||||||||
| ctx.gts.clear(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| for index in &dev.incremental_result.client_components_affected { | ||||||||||||||||||||||||||
| // `trace_dependencies` appends every boundary it visits to this same | ||||||||||||||||||||||||||
| // list (their trace bit is set at that point, so visiting them again | ||||||||||||||||||||||||||
| // below is a no-op), so it must not be iterated through a borrow. | ||||||||||||||||||||||||||
|
robobun marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| let mut i = 0; | ||||||||||||||||||||||||||
| while i < dev.incremental_result.client_components_affected.len() { | ||||||||||||||||||||||||||
| let index = dev.incremental_result.client_components_affected[i]; | ||||||||||||||||||||||||||
| dev.server_graph.trace_dependencies( | ||||||||||||||||||||||||||
| *index, | ||||||||||||||||||||||||||
| index, | ||||||||||||||||||||||||||
| ctx.gts, | ||||||||||||||||||||||||||
| incremental_graph::TraceDependencyGoal::NoStop, | ||||||||||||||||||||||||||
| *index, | ||||||||||||||||||||||||||
| index, | ||||||||||||||||||||||||||
| )?; | ||||||||||||||||||||||||||
| i += 1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| for request in &dev.incremental_result.framework_routes_affected { | ||||||||||||||||||||||||||
|
|
@@ -4984,6 +4994,42 @@ impl DevServer { | |||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// A "use client" file whose own imports failed to resolve. The failure | ||||||||||||||||||||||||||
| /// belongs to the client graph (the file was parsed for the browser, see | ||||||||||||||||||||||||||
| /// `get_log_for_resolution_failures`), but a boundary that bundles gets a | ||||||||||||||||||||||||||
| /// node on both sides: server-side importers attach their edges to the | ||||||||||||||||||||||||||
| /// server node, whose boundary flag makes import traces continue into the | ||||||||||||||||||||||||||
| /// client graph, and the client node is an HMR root so dependency traces | ||||||||||||||||||||||||||
| /// cross back to the server side. `finalize_bundle` only sets this up for | ||||||||||||||||||||||||||
| /// files that bundled, so do it here for the failed one; otherwise the | ||||||||||||||||||||||||||
| /// routes importing the file are never associated with its failure and get | ||||||||||||||||||||||||||
| /// served as if it had bundled. | ||||||||||||||||||||||||||
|
robobun marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| pub(crate) fn handle_client_component_boundary_failure( | ||||||||||||||||||||||||||
| &mut self, | ||||||||||||||||||||||||||
| abs_path: &[u8], | ||||||||||||||||||||||||||
| ) -> Result<(), AllocError> { | ||||||||||||||||||||||||||
| let _g = self.graph_safety_lock.guard(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // This only looks the client node up: it was created for the | ||||||||||||||||||||||||||
| // resolution log, and its failure (inserted once the bundle finishes) | ||||||||||||||||||||||||||
| // is what marks it stale. | ||||||||||||||||||||||||||
|
robobun marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| let client_index = self.client_graph.insert_stale(abs_path, false)?; | ||||||||||||||||||||||||||
| self.client_graph.bundled_files.values_mut()[client_index.get() as usize].is_hmr_root = | ||||||||||||||||||||||||||
| true; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Nothing gets bundled for the server node until the file is fixed, so | ||||||||||||||||||||||||||
| // it must stay stale (`insert_stale` alone cannot set the bit for an | ||||||||||||||||||||||||||
| // index past the bitset's current length). | ||||||||||||||||||||||||||
|
robobun marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| let server_index = self.server_graph.insert_stale(abs_path, false)?; | ||||||||||||||||||||||||||
| self.server_graph.ensure_stale_bit_capacity(true)?; | ||||||||||||||||||||||||||
| self.server_graph | ||||||||||||||||||||||||||
| .stale_files | ||||||||||||||||||||||||||
| .set(server_index.get() as usize); | ||||||||||||||||||||||||||
| self.server_graph.bundled_files.values_mut()[server_index.get() as usize] | ||||||||||||||||||||||||||
| .is_client_component_boundary = true; | ||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /// Return a log to write resolution failures into. | ||||||||||||||||||||||||||
| pub(crate) fn get_log_for_resolution_failures( | ||||||||||||||||||||||||||
| &mut self, | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.