[pull] canary from vercel:canary - #1394
Merged
Merged
Conversation
…ing an owned string (#98520) ### What? Follow-up to the review comments on #98497. `new_atom` loses its generic parameter and takes a `Cow<'_, str>`, which as a side effect stops it copying a string it was handed ownership of. `FileSystemPath`'s `get_relative_request_to` stops hand-rolling a conversion the standard `From` impl already does, and the two tests added for it in `path.rs` are replaced by one that covers what that layer actually adds. ### Why? @bgw noted that `get_relative_request_to` was matching on the `Cow` it gets back only to call `.into()` in both arms, and wondered whether `impl From<Cow<'_, str>> for RcStr` was missing a fast path for the owned case. It was not missing one, and it never needed one — but an owned string *was* being copied, for a different reason. `new_atom` was generic over `T: AsRef<str> + Into<String>` and opened with `let text = text.as_ref();`, which shadows the owned value. From that point on only a `&str` was in scope, so building the `Box<str>` allocated a fresh buffer and copied into it, dropping the `String` that a `Cow::Owned` had just handed over. So this is not a fast path being added. It is an accidental pessimisation being removed: the generic plus the `as_ref()` were exactly what stopped the standard conversion from doing the right thing on its own. Whether that copy ever mattered in practice is a separate question, and one this does not attempt to answer. The other two comments were about the tests: they re-ran the case table that already lives in `turbo-unix-path`, which is where the path computation itself is implemented and tested — "60 LOC to test a 3 LOC branch". ### How? `new_atom` takes a `Cow<'_, str>`. All five of its callers already passed one, so nothing else changes, and ownership now survives to where the `Box<str>` is built — `Box<str>`'s own `From<Cow<'_, str>>` moves an owned buffer in and copies only a borrowed one. The inline-atom path is untouched, since it reads through a `&str` either way and does not allocate. Contents-based assertions cannot distinguish a moved buffer from a copied one, so the test compares the data pointer before and after the conversion. It uses a string longer than `MAX_INLINE_LEN`, because an inline atom stores its bytes in the tagged value by design and could never preserve a pointer, and it asserts the atom really is dynamic so it cannot pass vacuously if that boundary ever moves. It fails on `canary` today. `get_relative_request_to` collapses to a single `.into()`. Its sibling `get_relative_path_to` keeps its match on purpose: one arm uses `std::ptr::eq` to hand back a clone of the `RcStr` the path already holds, which the blanket conversion cannot do. That arm predates #98497 and is an optimisation rather than the redundancy that was flagged. For the tests, `turbo-unix-path` remains the place where the relative path computation is covered, untouched. What is left here is only what this layer adds on top: that each method reaches for the form it names, and that neither returns a path across filesystems. Keeping the first of those is a judgement call — a method delegating to the wrong free function is precisely the bug #98497 existed to fix, and it is invisible to the lower-level table — so it is asserted with one descendant pair rather than a table. Happy to drop it entirely if you would rather. To check that the smaller test still earns its place, `get_relative_request_to` was temporarily pointed at the plain-path function; the test fails as it should. <!-- NEXT_JS_LLM --> <!-- fleet 3acc7e63-52a8-4472-89cd-b1bbb8ad59fa --> --------- Co-authored-by: vercel-fleet-prod[bot] <318278635+vercel-fleet-prod[bot]@users.noreply.github.com> Co-authored-by: Tobias Koppers <1365881+sokra@users.noreply.github.com>
### Problem 1. Next computes the warmup cache key while `SOME_ENV_VAR` is unset. 2. Rendering ends up setting `SOME_ENV_VAR`. 3. Final prerender expects the cache entry to exist, but the env var changed, so the key changed and it's a miss. 4. Unexpected miss and bailout. This lead to ``` Error: Route "foo": Unexpected cache miss after cache warming phase during prerendering. This is likely caused by non-deterministic arguments that differ between the cache warming phase and the final prerender phase (e.g. unstable array order). Ensure that arguments passed to cached functions are deterministic. Error: Route "foo": Next.js encountered uncached or runtime data during prerendering. ``` ### Solution ~~Instead, snapshot the env vars once at module evaluation time of the `use cache` function. This ensures that the cache key doesn't change over time. This is also how it worked thus far (with deployment id as the cache key): changing the env var over the lifetime of the process didn't lead to a reexecution of the `use cache` function~~ RDC already stores cache entries generated in the previous phase (be it during `next build` multi-phase rendering, or from prerender->resuming at runtime). Root params are already excluded from the cache keys when storing in RDC. Also exclude the env var hash bit, to conform to this system of preventing tearing (at the cost of potential staleness).
### What? Globally order all emitted Turbopack ECMAScript chunk items by module path, including items nested inside batches. Browser and Node.js chunk emitters now consume the same flattened, ordered result. ### Why? The previous ordering compared only the first module in each item or batch group. Remaining modules in a batch stayed together even when their paths belonged elsewhere in the global order, reducing locality between similar modules and slightly worsening gzip compression. For `bench/basic-app`, production JavaScript chunk gzip sizes changed as follows while raw bytes and chunk counts stayed unchanged: | Output | Before | After | Change | |---|---:|---:|---:| | Browser chunks | 1,619,643 B | 1,619,314 B | -329 B (-0.020%) | | Server chunks | 792,379 B | 792,186 B | -193 B (-0.024%) | | Combined | 2,412,022 B | 2,411,500 B | -522 B (-0.022%) | ### How? The shared ECMAScript chunk-content accessor now flattens resolved item and batch groups and sorts every module by `(path, module id)`. Keeping this ordering in the shared layer gives browser and Node.js output identical deterministic behavior and removes duplicated emitter-side sorting. ### Verification - `cargo fmt --all -- --check` - `cargo check -p turbopack-browser -p turbopack-nodejs` - `cargo test -p turbopack-browser -p turbopack-nodejs` - `pnpm build-all` - Clean baseline and candidate production builds of `bench/basic-app` <!-- NEXT_JS_LLM --> <!-- fleet d30d9e3f-6885-4665-ae08-b37fa6151b95 --> --------- Co-authored-by: vercel-fleet-prod[bot] <318278635+vercel-fleet-prod[bot]@users.noreply.github.com> Co-authored-by: Tobias Koppers <1365881+sokra@users.noreply.github.com>
### What? Reduce Turbopack ECMAScript chunk size by emitting strict module factories in a shared strict-mode scope instead of repeating `"use strict"` in every factory. The chunk format now chooses among the existing flat representation, a mixed representation with flat non-strict factories plus a nested strict array, and an all-strict representation that wraps the complete chunk in a strict IIFE. Each factory is generated through the existing single code path; the production minimizer removes its redundant directive when the factory is created inside a strict wrapper. Browser and Node.js emitters share the mode selection and serialization implementation, while the runtime remains compatible with the existing flat format. ### Why? ECMAScript modules are always strict, so large chunks currently repeat the same directive across many factories. Moving strictness to the scope where those factories are created removes redundant bytes without copying arrays or changing strict/sloppy execution semantics. For `bench/basic-app`, this reduces total raw JavaScript by 16,831 bytes (0.161%). Partitioning factories changes compression behavior, resulting in a 1,286-byte (0.051%) gzip increase across all JavaScript; format selection therefore remains based on emitted raw bytes. ### How? - Determine each chunk item's strictness from the already-collected chunk metadata. - Generate every factory once with its normal directive, then rely on the minimizer to remove redundant directives inside strict wrappers. - Compare the expected minified directive bytes removed with the exact wrapper bytes added and keep small chunks in the existing flat format. - Keep non-strict factories flat in mixed chunks and append strict factories as an array produced by a strict IIFE. - Wrap the complete registration/export in a strict IIFE when every factory is strict, keeping those factories flat and avoiding a nested array entirely. - Use the shorter arrow IIFE when the target supports it, with a function IIFE fallback for older targets. - Preserve source-map sections, scope-hoisted module IDs, factory naming, and ordering within each partition. - Add focused strict, sloppy, mixed, all-strict, old-target, and below-threshold coverage and update affected Turbopack snapshots. ### Verification - `cargo nextest run -p turbopack-tests -E 'test(snapshot)'` - `cargo clippy -p turbopack-ecmascript -p turbopack-browser -p turbopack-nodejs` - `pnpm build-all` - `pnpm --dir turbopack/crates/turbopack-ecmascript-runtime/js check:nodejs` - `bench/basic-app` production Turbopack build, repeated to confirm deterministic size totals <!-- NEXT_JS_LLM --> <!-- fleet 528ffe69-eb27-466b-a132-842272a782cf --> --------- Co-authored-by: vercel-fleet-prod[bot] <318278635+vercel-fleet-prod[bot]@users.noreply.github.com> Co-authored-by: Tobias Koppers <1365881+sokra@users.noreply.github.com>
…pgrade (#98560) ### Why? Async Request API codemods can leave temporary casts that still require application-specific repairs. Those locations need explicit markers, while optional feature adoption needs to be distinguishable from required version migrations. ### How? Add `@next-codemod-error` comments to temporary `UnsafeUnwrapped*` casts, with instructions for completing the repair. Preserve documented ignores and avoid duplicating casts or markers when the transform runs again. Mark generated Cache Components opt-outs with `@next-codemod-ignore`, a removal condition and a link to the adoption guide. Add `upgrade --skip-adoption` to skip the Cache Components `instant = false` transform and partial-prefetch adoption cleanup. Required migrations and normal dependency and bundler selection keep their existing behavior. Both adoption transforms can still be run explicitly.
Stacked on #98560. ### Why? Users need to distinguish unfinished codemod repairs from intentional feature-adoption opt-outs. ### How? Document `@next-codemod-error` and temporary `UnsafeUnwrapped*` casts in the codemod guide, with examples and a link to the Async Request APIs migration guide. Explain when an inapplicable suggestion can be marked `@next-codemod-ignore` with a reason. Show the ignore comment emitted for Cache Components opt-outs and explain that it does not block compilation.
### What? Prevent the OpenTelemetry instrumentation e2e collector from losing span exports when an exporter reuses an idle localhost connection. Add focused coverage for the collector's connection behavior. ### Why? Keeping the collector listener alive for each application's full lifetime removed the earlier close-and-rebind gap, but did not eliminate every socket failure. Node's HTTP server could still time out an idle keep-alive socket retained by the exporter's Undici pool. A later export could race that closure, lose spans with `UND_ERR_SOCKET` or `ECONNRESET`, and leave otherwise unrelated trace assertions with incomplete trees. The canary.28 failure was investigated separately and is not this mechanism: it occurred in the release-enabled Rspack matrix when a custom server exited during startup. This change is limited to the residual non-Rspack collector race and does not alter Rspack coverage or release snapshots. ### How? Successful collector exports explicitly close their HTTP connection. Each export therefore uses a fresh local connection instead of retaining a socket that the server may reclaim while idle. The regression test asserts this response contract directly, avoiding timing-dependent coverage of the race. ### Verification - Node 20.9.0, webpack dev, React 18.3.1: 5 consecutive focused runs, 55 tests passed per run - Node 20.9.0, webpack start, React 18.3.1: 99 passed, 13 existing skips - `pnpm test-types` - Prettier and ESLint on changed files <!-- NEXT_JS_LLM --> <!-- fleet e4b8ed15-4a06-4727-a634-fddcc27c11fc --> Co-authored-by: vercel-fleet-prod[bot] <318278635+vercel-fleet-prod[bot]@users.noreply.github.com> Co-authored-by: Tobias Koppers <1365881+sokra@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )