Fix TanStack Start compatibility#5756
Draft
TheHypnoo wants to merge 5 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates HIR lowering for hoisted TextEncoder/TextDecoder typing, JSX and dynamic import handling, adjusts runtime fetch and Web Streams behavior, and changes package subpath resolution with new regression tests. ChangesHIR lowering and JSX/import handling
Runtime fetch, streams, and superclass dispatch
Package subpath export resolution
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/compile/resolve.rs (1)
599-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
.tsxand.mtsmirrors here too.
resolve_with_extensionsprefers.ts,.tsx, and.mts, but this helper only probes.ts. A compile-package export that resolves todist/foo.jswith source atsrc/foo.tsxorsrc/foo.mtswill still stay on JS here, so the subpath misses native source resolution.Proposed fix
fn prefer_ts_source_for_package_entry( package_dir: &Path, normal_entry: PathBuf, ) -> Option<PathBuf> { if is_js_file(&normal_entry) { - // Try .ts equivalent of the .js entry - let ts_path = normal_entry.with_extension("ts"); - if ts_path.exists() { - return Some(ts_path); + for ext in ["ts", "tsx", "mts"] { + let ts_path = normal_entry.with_extension(ext); + if ts_path.exists() && ts_path.is_file() { + return Some(ts_path); + } } // Check src/ directory mirror of lib/ or dist/ path if let Ok(rel) = normal_entry.strip_prefix(package_dir) { let rel_str = rel.to_string_lossy(); if rel_str.starts_with("lib") || rel_str.starts_with("dist") { @@ }; if let Ok(rest) = stripped { - let src_equiv = package_dir.join("src").join(rest).with_extension("ts"); - if src_equiv.exists() { - return Some(src_equiv); + for ext in ["ts", "tsx", "mts"] { + let src_equiv = package_dir.join("src").join(rest).with_extension(ext); + if src_equiv.exists() && src_equiv.is_file() { + return Some(src_equiv); + } } } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/resolve.rs` around lines 599 - 628, Update prefer_ts_source_for_package_entry so it probes native TypeScript mirrors for .js package entries, not just .ts. When is_js_file(normal_entry) is true, check the .ts, .tsx, and .mts equivalents of the entry, and also the src mirror for lib/dist paths using the same set of extensions. Keep the existing control flow in prefer_ts_source_for_package_entry and ensure resolve_with_extensions stays aligned with the source-file preference logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-hir/src/lower/expr_function.rs`:
- Around line 779-789: The hoisted type inference in
infer_hoisted_text_codec_var_type only checks builtin aliases recorded in the
current var pre-pass, so util references from outer scope still resolve to Any.
Update the logic in expr_function.rs to consult LoweringContext for util aliases
as well, matching the module-level handling by using
ctx.lookup_builtin_module_alias and ctx.lookup_native_module alongside the
existing builtin_aliases_in_var_decl predicate. Keep the current var-declaration
alias tracking, but widen the check around decl.init / ident handling so util
and node:util are recognized even when imported or required outside the local
pre-pass.
---
Outside diff comments:
In `@crates/perry/src/commands/compile/resolve.rs`:
- Around line 599-628: Update prefer_ts_source_for_package_entry so it probes
native TypeScript mirrors for .js package entries, not just .ts. When
is_js_file(normal_entry) is true, check the .ts, .tsx, and .mts equivalents of
the entry, and also the src mirror for lib/dist paths using the same set of
extensions. Keep the existing control flow in prefer_ts_source_for_package_entry
and ensure resolve_with_extensions stays aligned with the source-file preference
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 211f40fc-7814-4057-b8e0-94a47b7ac54f
📒 Files selected for processing (7)
crates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower_types.rscrates/perry/src/commands/compile/resolve.rscrates/perry/src/commands/compile/resolve/tests.rstests/test_compile_package_exports_subpath_source.shtests/test_textencoder_hoisted_function_decl.sh
TheHypnoo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-stdlib/src/fetch/request_ctor.rs (1)
50-56: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject GET/HEAD stream bodies before draining them.
Line 50 drains a pending
ReadableStreambefore the Line 55 GET/HEAD guard. A lazy stream can be consumed or block even though the constructor is about to throw.Defer draining until after the method/body validation
- let body: Option<Vec<u8>> = take_pending_fetch_body_stream_id() - .map(crate::streams::drain_readable_into_bytes) - .or_else(|| dispatch::body_addr_buffer_bytes(body_ptr as usize)) - .or_else(|| dispatch::body_bytes_from_header(body_ptr)); + let pending_stream_id = take_pending_fetch_body_stream_id(); + let non_stream_body = if pending_stream_id.is_none() { + dispatch::body_addr_buffer_bytes(body_ptr as usize) + .or_else(|| dispatch::body_bytes_from_header(body_ptr)) + } else { + None + }; // GET/HEAD requests may not carry a body (WHATWG fetch). Refs `#2643`. - if body.is_some() && (method == "GET" || method == "HEAD") { + if (pending_stream_id.is_some() || non_stream_body.is_some()) + && (method == "GET" || method == "HEAD") + { throw_fetch_type_error("Request with GET/HEAD method cannot have body."); } + let body = pending_stream_id + .map(crate::streams::drain_readable_into_bytes) + .or(non_stream_body);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-stdlib/src/fetch/request_ctor.rs` around lines 50 - 56, The Request constructor logic in request_ctor should validate the method before consuming any pending body stream. Move the GET/HEAD body check ahead of the take_pending_fetch_body_stream_id/drain_readable_into_bytes path so a lazy ReadableStream is not drained or blocked when the constructor will throw anyway. Keep the existing body sources and throw_fetch_type_error behavior intact, but ensure the method/body validation gates access to stream draining in this constructor flow.crates/perry-stdlib/src/fetch/mod.rs (1)
1333-1355: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t silently drop stream-backed bodies when cloning.
For
new Response(readableStream),resp.bodyis empty and the payload lives only inresp.body_stream_id. This clone keepsbody_present: truebut setsbody_stream_id: None, so the clone exposes an empty body. Either tee/copy the stream state or throw until stream teeing is supported.Minimal guard to avoid silent data loss
guard.get(&id).map(|resp| { if resp.body_present && resp.body_used { unsafe { throw_fetch_type_error("Response.clone: Body has already been consumed.") }; } + if resp.body_stream_id.is_some() { + unsafe { + throw_fetch_type_error("Response.clone: streaming bodies are not yet cloneable.") + }; + } FetchResponse {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-stdlib/src/fetch/mod.rs` around lines 1333 - 1355, js_response_clone currently clones stream-backed responses incorrectly by copying only resp.body and clearing body_stream_id, which makes Response(readableStream) clones lose their payload. Update the cloning logic in js_response_clone/FetchResponse so stream-backed bodies are either properly tee’d/copied along with body_stream_id (and related stream state) or, if that isn’t supported yet, throw a type error instead of returning an empty clone. Use the existing FetchResponse fields and the body_present/body_used checks to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/extern_func.rs`:
- Around line 1353-1356: The JSX fast-path in ExternFuncRef handling is only
excluding ctx.imported_vars, so imported cross-module jsx/jsxs calls can still
be rewritten incorrectly. Update the guard in lower_call/extern_func.rs around
try_rewrite_perry_tui_jsx_intrinsic to also skip names that are imported via
ctx.import_function_prefixes, ensuring imported jsx/jsxs bindings are not routed
to js_jsx/js_jsxs but left as normal imported calls.
In `@crates/perry-hir/src/lower/lower_module_fn.rs`:
- Around line 30-39: The React import detection in lower_module_fn currently
treats all imports from "react" as runtime imports, so type-only React imports
still set has_explicit_react_import. Update the import scan in the
lower_module_fn logic to ignore type-only imports by checking import.type_only
and any named specifiers’ is_type_only flags before marking React as available,
so only real runtime React imports prevent the synthetic namespace import.
In `@crates/perry-hir/src/lower/module_decl.rs`:
- Around line 425-433: The React namespace import handling in module_decl.rs is
currently binding type-only imports into `react_default_import_local`, which can
leave JSX lowering pointing at a runtime React symbol that does not exist.
Update the `source == "react"` branch in the import lowering logic to skip this
assignment when `whole_decl_type_only` is true, so only real runtime React
namespace imports populate `ctx.react_default_import_local`. Keep the existing
behavior for non-type-only React namespace imports and leave the JSX lowering
path unchanged otherwise.
In `@crates/perry-runtime/src/object/field_set_by_name.rs`:
- Around line 396-418: Move the Web Streams setter handling in
`field_set_by_name` so it runs before any early primitive-value return path; the
current `f64::from_bits`/`stream_handle_probe` block can exit before
`handle_property_set_dispatch` is invoked for finite numeric stream ids. Keep
the same stream-id detection logic, but ensure expando writes like
`stream.allReady = ...` reach the dispatch callback instead of being treated as
plain numbers.
In `@crates/perry-runtime/src/object/global_this/fetch_globals.rs`:
- Around line 507-545: The helper is out of sync with the builtin class-ID
mapping: `is_uncallable_builtin_super_parent_class_id` includes entries that
`global_builtin_constructor_class_id` cannot resolve for `WeakMap`, `WeakSet`,
`SharedArrayBuffer`, and `DataView`. Update the builtin ID handling so these
constructors are recognized consistently, or remove the unreachable names from
the `NAMES` list, ensuring the logic in
`is_uncallable_builtin_super_parent_class_id` matches the IDs returned by
`global_builtin_constructor_class_id`.
In `@crates/perry/src/commands/compile/run_pipeline.rs`:
- Around line 2765-2768: The named-import resolution in the compile pipeline is
bypassing the function-declaration filter by checking
source_module.exported_objects directly, which can reintroduce exported function
names that exported_var_names intentionally excludes. Update the import/export
matching logic in run_pipeline so the immediate source-module path uses
exported_var_names or applies the same is_function_decl exclusion before
treating a symbol as a variable export. Keep the fix localized to the code that
decides whether origin_key is considered exported for named imports, so
callback/value imports don’t get miscompiled as variable imports.
---
Outside diff comments:
In `@crates/perry-stdlib/src/fetch/mod.rs`:
- Around line 1333-1355: js_response_clone currently clones stream-backed
responses incorrectly by copying only resp.body and clearing body_stream_id,
which makes Response(readableStream) clones lose their payload. Update the
cloning logic in js_response_clone/FetchResponse so stream-backed bodies are
either properly tee’d/copied along with body_stream_id (and related stream
state) or, if that isn’t supported yet, throw a type error instead of returning
an empty clone. Use the existing FetchResponse fields and the
body_present/body_used checks to locate the fix.
In `@crates/perry-stdlib/src/fetch/request_ctor.rs`:
- Around line 50-56: The Request constructor logic in request_ctor should
validate the method before consuming any pending body stream. Move the GET/HEAD
body check ahead of the
take_pending_fetch_body_stream_id/drain_readable_into_bytes path so a lazy
ReadableStream is not drained or blocked when the constructor will throw anyway.
Keep the existing body sources and throw_fetch_type_error behavior intact, but
ensure the method/body validation gates access to stream draining in this
constructor flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc637be5-c979-4a8c-83f6-f465ebf59911
📒 Files selected for processing (20)
crates/perry-codegen/src/lower_call/extern_func.rscrates/perry-hir/src/dynamic_import/visitors.rscrates/perry-hir/src/jsx.rscrates/perry-hir/src/lower/expr_function.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/global_this/fetch_globals.rscrates/perry-runtime/src/object/global_this/proto_methods.rscrates/perry-runtime/src/proxy/put_value.rscrates/perry-runtime/src/url/abort.rscrates/perry-stdlib/src/common/dispatch/property_dispatch.rscrates/perry-stdlib/src/fetch/abort_bridge.rscrates/perry-stdlib/src/fetch/body_metadata.rscrates/perry-stdlib/src/fetch/dispatch.rscrates/perry-stdlib/src/fetch/mod.rscrates/perry-stdlib/src/fetch/request_ctor.rscrates/perry/src/commands/compile/run_pipeline.rscrates/perry/tests/issue_5756_response_stream_body.rs
✅ Files skipped from review due to trivial changes (1)
- crates/perry-stdlib/src/fetch/body_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-hir/src/lower/expr_function.rs
Comment on lines
1353
to
1356
| "jsx" | "jsxs" if !ctx.imported_vars.contains(name) => { | ||
| if let Some(call) = try_rewrite_perry_tui_jsx_intrinsic(ctx, name == "jsxs", args)? { | ||
| return Ok(Some(call)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Also exclude imported functions from the JSX fast-path.
ExternFuncRef is used for cross-module calls too, and an imported function named jsx/jsxs will usually be in ctx.import_function_prefixes, not ctx.imported_vars. With the current guard, those calls still route to js_jsx/js_jsxs instead of the imported binding.
Proposed fix
- "jsx" | "jsxs" if !ctx.imported_vars.contains(name) => {
+ "jsx" | "jsxs"
+ if !ctx.imported_vars.contains(name)
+ && !ctx.import_function_prefixes.contains_key(name)
+ && !ctx.import_function_v8_specifiers.contains_key(name) =>
+ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "jsx" | "jsxs" if !ctx.imported_vars.contains(name) => { | |
| if let Some(call) = try_rewrite_perry_tui_jsx_intrinsic(ctx, name == "jsxs", args)? { | |
| return Ok(Some(call)); | |
| } | |
| "jsx" | "jsxs" | |
| if !ctx.imported_vars.contains(name) | |
| && !ctx.import_function_prefixes.contains_key(name) | |
| && !ctx.import_function_v8_specifiers.contains_key(name) => | |
| { | |
| if let Some(call) = try_rewrite_perry_tui_jsx_intrinsic(ctx, name == "jsxs", args)? { | |
| return Ok(Some(call)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/lower_call/extern_func.rs` around lines 1353 - 1356,
The JSX fast-path in ExternFuncRef handling is only excluding ctx.imported_vars,
so imported cross-module jsx/jsxs calls can still be rewritten incorrectly.
Update the guard in lower_call/extern_func.rs around
try_rewrite_perry_tui_jsx_intrinsic to also skip names that are imported via
ctx.import_function_prefixes, ensuring imported jsx/jsxs bindings are not routed
to js_jsx/js_jsxs but left as normal imported calls.
Comment on lines
+30
to
+39
| let mut has_explicit_react_import = false; | ||
| let mut has_react_ecosystem_import = false; | ||
| for item in &ast_module.body { | ||
| let ast::ModuleItem::ModuleDecl(ast::ModuleDecl::Import(import)) = item else { | ||
| continue; | ||
| }; | ||
| let source = import.src.value.to_string_lossy().to_string(); | ||
| if source == "react" { | ||
| has_explicit_react_import = true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --glob '*.{rs,sh,ts,tsx,js,jsx}' \
'import\s+type\b.*from\s+["'\''"]react["'\''"]|from\s+["'\''"]react["'\''"].*\btype\b' .Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the lowering logic.
ast-grep outline crates/perry-hir/src/lower/lower_module_fn.rs --view expanded || true
wc -l crates/perry-hir/src/lower/lower_module_fn.rs
sed -n '1,120p' crates/perry-hir/src/lower/lower_module_fn.rs
# Inspect the AST import fields used by this code path.
rg -n "type_only|ImportSpecifier::Named|ImportSpecifier::Default|ImportSpecifier::Namespace|has_explicit_react_import|react_ecosystem" crates/perry-hir crates -g '*.rs'Repository: PerryTS/perry
Length of output: 25056
Ignore type-only React imports here. import type ... from "react" still flips has_explicit_react_import, so TSX files with only React type imports skip the synthetic react namespace import and can fall back to the eager JSX path. Check import.type_only and named.is_type_only before treating React as available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-hir/src/lower/lower_module_fn.rs` around lines 30 - 39, The
React import detection in lower_module_fn currently treats all imports from
"react" as runtime imports, so type-only React imports still set
has_explicit_react_import. Update the import scan in the lower_module_fn logic
to ignore type-only imports by checking import.type_only and any named
specifiers’ is_type_only flags before marking React as available, so only real
runtime React imports prevent the synthetic namespace import.
Comment on lines
+425
to
+433
| // React namespace imports are the common TSX shape | ||
| // (`import * as React from "react"`). They need the | ||
| // same non-eager React element semantics as default | ||
| // React imports; Perry's native JSX adapter calls | ||
| // function components immediately and therefore runs | ||
| // hooks outside the reconciler. | ||
| if source == "react" { | ||
| ctx.react_default_import_local = Some(local.clone()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --glob '*.{rs,sh,ts,tsx,js,jsx}' \
'import\s+type\s+\*\s+as\s+\w+\s+from\s+["'\''"]react["'\''"]' .Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and surrounding code.
FILE="crates/perry-hir/src/lower/module_decl.rs"
echo "== Outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant lines around 360-470 =="
sed -n '360,470p' "$FILE" | cat -n
echo
echo "== Search for whole_decl_type_only usage =="
rg -n 'whole_decl_type_only|react_default_import_local|namespace import|import type \* as' "$FILE" .Repository: PerryTS/perry
Length of output: 50369
Exclude type-only React namespace imports from the runtime JSX binding. import type * as React from "react" is erased, but this branch still stores react_default_import_local, so JSX can lower to React.createElement(...) without a runtime React binding. Gate the React assignment on !whole_decl_type_only.
Proposed fix
- if source == "react" {
+ if source == "react" && !whole_decl_type_only {
ctx.react_default_import_local = Some(local.clone());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // React namespace imports are the common TSX shape | |
| // (`import * as React from "react"`). They need the | |
| // same non-eager React element semantics as default | |
| // React imports; Perry's native JSX adapter calls | |
| // function components immediately and therefore runs | |
| // hooks outside the reconciler. | |
| if source == "react" { | |
| ctx.react_default_import_local = Some(local.clone()); | |
| } | |
| // React namespace imports are the common TSX shape | |
| // (`import * as React from "react"`). They need the | |
| // same non-eager React element semantics as default | |
| // React imports; Perry's native JSX adapter calls | |
| // function components immediately and therefore runs | |
| // hooks outside the reconciler. | |
| if source == "react" && !whole_decl_type_only { | |
| ctx.react_default_import_local = Some(local.clone()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-hir/src/lower/module_decl.rs` around lines 425 - 433, The React
namespace import handling in module_decl.rs is currently binding type-only
imports into `react_default_import_local`, which can leave JSX lowering pointing
at a runtime React symbol that does not exist. Update the `source == "react"`
branch in the import lowering logic to skip this assignment when
`whole_decl_type_only` is true, so only real runtime React namespace imports
populate `ctx.react_default_import_local`. Keep the existing behavior for
non-type-only React namespace imports and leave the JSX lowering path unchanged
otherwise.
Comment on lines
+396
to
+418
| // #5756: Web Streams handles are represented as finite f64 ids in the | ||
| // stream-id band (not NaN-boxed pointers). Reads already route those ids | ||
| // through `handle_property_dispatch`; writes need the matching setter path | ||
| // so userland/React can attach expando fields like `stream.allReady`. | ||
| { | ||
| let f = f64::from_bits(obj as u64); | ||
| if !key.is_null() && f.is_finite() && f > 0.0 && f.fract() == 0.0 { | ||
| let id = f as usize; | ||
| if let Some(probe) = crate::object::stream_handle_probe() { | ||
| unsafe { | ||
| if probe(id) { | ||
| if let Some(dispatch) = handle_property_set_dispatch() { | ||
| let name_ptr = | ||
| (key as *const u8).add(std::mem::size_of::<crate::StringHeader>()); | ||
| let name_len = (*key).byte_len as usize; | ||
| dispatch(id as i64, name_ptr, name_len, value); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) How does is_number classify a raw finite f64 (a stream id)?
rg -nP 'fn is_number' --type rust -A8
# 2) What numeric band do stream handle ids occupy?
rg -nP 'STREAM_HANDLE_ID_(START|END)' --type rust -A1
# 3) Confirm the primitive-number guard precedes the new stream block in the set path.
rg -nP 'jv\.is_number\(\)\s*&&\s*top16' crates/perry-runtime/src/object/field_set_by_name.rsRepository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant guard and stream-handle logic in the target file
ast-grep outline crates/perry-runtime/src/object/field_set_by_name.rs --view expanded
# Find the exact primitive-number guard and the new stream-handle write block
rg -n -C 6 'is_number\(\)|handle_property_set_dispatch|stream_handle_probe|top16|fract\(\)' crates/perry-runtime/src/object/field_set_by_name.rs
# Find definitions/usages of the stream-handle range and number classification
rg -n -C 4 'stream handle|STREAM_HANDLE|is_number|is_primitive_number|handle_property_dispatch|handle_property_set_dispatch' crates/perry-runtime/src -g '*.rs'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow reads for the exact semantics in play
sed -n '60,95p' crates/perry-runtime/src/value/jsvalue.rs
sed -n '1,120p' crates/perry-runtime/src/value/addr_class.rs
sed -n '1,140p' crates/perry-runtime/src/object/class_handles.rs
# Inspect the write-path guards around the new stream-handle block
sed -n '330,460p' crates/perry-runtime/src/object/field_set_by_name.rsRepository: PerryTS/perry
Length of output: 19939
Hoist the Web Streams setter block above the primitive-value return. Raw stream ids are plain numbers, so this guard returns before handle_property_set_dispatch can run and stream.allReady = … is dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/field_set_by_name.rs` around lines 396 - 418,
Move the Web Streams setter handling in `field_set_by_name` so it runs before
any early primitive-value return path; the current
`f64::from_bits`/`stream_handle_probe` block can exit before
`handle_property_set_dispatch` is invoked for finite numeric stream ids. Keep
the same stream-id detection logic, but ensure expando writes like
`stream.allReady = ...` reach the dispatch callback instead of being treated as
plain numbers.
Comment on lines
+507
to
+545
| fn is_uncallable_builtin_super_parent_class_id(class_id: u32) -> bool { | ||
| if class_id == 0 { | ||
| return false; | ||
| } | ||
| const NAMES: &[&str] = &[ | ||
| "Map", | ||
| "Set", | ||
| "WeakMap", | ||
| "WeakSet", | ||
| "Array", | ||
| "ArrayBuffer", | ||
| "SharedArrayBuffer", | ||
| "DataView", | ||
| "Boolean", | ||
| "Number", | ||
| "String", | ||
| "Date", | ||
| "RegExp", | ||
| "Promise", | ||
| "Function", | ||
| "BigInt", | ||
| "Symbol", | ||
| "Object", | ||
| "Int8Array", | ||
| "Uint8Array", | ||
| "Uint8ClampedArray", | ||
| "Int16Array", | ||
| "Uint16Array", | ||
| "Int32Array", | ||
| "Uint32Array", | ||
| "Float32Array", | ||
| "Float64Array", | ||
| "BigInt64Array", | ||
| "BigUint64Array", | ||
| ]; | ||
| NAMES | ||
| .iter() | ||
| .any(|name| super::super::instanceof::global_builtin_constructor_class_id(name) == class_id) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which of the listed names global_builtin_constructor_class_id resolves to 0.
rg -nP 'fn global_builtin_constructor_class_id' --type rust -A50 \
| rg -nP 'WeakMap|WeakSet|SharedArrayBuffer|DataView|_ =>'Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for rel, start, end in [
('crates/perry-runtime/src/object/global_this/fetch_globals.rs', 450, 560),
('crates/perry-runtime/src/object/instanceof.rs', 1, 260),
]:
p = Path(rel)
lines = p.read_text().splitlines()
print(f"== {rel} {start}-{end} ==")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}: {lines[i]}")
print()
PYRepository: PerryTS/perry
Length of output: 17573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/perry-runtime/src/object/instanceof.rs')
lines = p.read_text().splitlines()
for marker in ['fn global_builtin_constructor_class_id', 'WeakMap', 'SharedArrayBuffer', '_ => 0']:
print(f"== {marker} ==")
for i, line in enumerate(lines, 1):
if marker in line:
for j in range(max(1, i-12), min(len(lines), i+35)+1):
print(f"{j}: {lines[j-1]}")
print('---')
PYRepository: PerryTS/perry
Length of output: 8407
Add the missing class IDs for these builtins
global_builtin_constructor_class_id never returns a non-zero id for WeakMap, WeakSet, SharedArrayBuffer, or DataView, so those entries in is_uncallable_builtin_super_parent_class_id can never match. That leaves the two guards out of sync and can send those parents down the wrong super() path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/object/global_this/fetch_globals.rs` around lines
507 - 545, The helper is out of sync with the builtin class-ID mapping:
`is_uncallable_builtin_super_parent_class_id` includes entries that
`global_builtin_constructor_class_id` cannot resolve for `WeakMap`, `WeakSet`,
`SharedArrayBuffer`, and `DataView`. Update the builtin ID handling so these
constructors are recognized consistently, or remove the unreachable names from
the `NAMES` list, ensuring the logic in
`is_uncallable_builtin_super_parent_class_id` matches the IDs returned by
`global_builtin_constructor_class_id`.
Comment on lines
+2765
to
+2768
| let source_exports_object = source_module | ||
| .is_some_and(|m| m.exported_objects.iter().any(|n| n == &exported_name)); | ||
| if source_exports_object | ||
| || exported_var_names.contains(&origin_key) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't bypass the function-declaration filter here.
exported_var_names intentionally excludes exported function foo(){} bindings even when they also flow through exported_objects (see Lines 717-731), because imported_vars switches codegen to getter/value semantics. This new raw source_module.exported_objects probe reintroduces those function names, so a named import used as a value/callback can be miscompiled as a variable import. Reuse exported_var_names for the immediate source path, or mirror the is_function_decl exclusion here.
Suggested fix
- let source_exports_object = source_module
- .is_some_and(|m| m.exported_objects.iter().any(|n| n == &exported_name));
+ let source_exports_object =
+ exported_var_names.contains(&(resolved_path_str.clone(), exported_name.clone()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let source_exports_object = source_module | |
| .is_some_and(|m| m.exported_objects.iter().any(|n| n == &exported_name)); | |
| if source_exports_object | |
| || exported_var_names.contains(&origin_key) | |
| let source_exports_object = | |
| exported_var_names.contains(&(resolved_path_str.clone(), exported_name.clone())); | |
| if source_exports_object | |
| || exported_var_names.contains(&origin_key) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry/src/commands/compile/run_pipeline.rs` around lines 2765 - 2768,
The named-import resolution in the compile pipeline is bypassing the
function-declaration filter by checking source_module.exported_objects directly,
which can reintroduce exported function names that exported_var_names
intentionally excludes. Update the import/export matching logic in run_pipeline
so the immediate source-module path uses exported_var_names or applies the same
is_function_decl exclusion before treating a symbol as a variable export. Keep
the fix localized to the code that decides whether origin_key is considered
exported for named imports, so callback/value imports don’t get miscompiled as
variable imports.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Fix TanStack Start compatibility for compiled package subpaths, React/TanStack TSX lowering, lazy route loading, and SSR response streams.
Changes
.ts,.tsx, and.mtssource mirrors for JavaScript package entries, includinglib/anddist/mirrors undersrc/.utilaliases.react/jsx-runtimecalls distinct from Perry native JSX lowering.new Response(ReadableStream)bodies lazily so pull-driven SSR streams can be consumed by downstream readers.Related issue
n/a
Test plan
cargo fmt --all -- --check./scripts/check_file_size.shcargo check -p perry-hircargo build -p perrycargo test -p perry --test issue_5756_response_stream_body -- --nocapturecargo test -p perry --test issue_5174_headers_http_pump_hang -- --nocapturecargo test -p perry compile_package_subpath_exports_do_not_fall_back_to_src_indextests/test_compile_package_exports_subpath_source.shtests/test_textencoder_hoisted_function_decl.shCompiled and served a minimal TanStack Start template through a Perry-compiled HTTP wrapper;
GET /returned HTTP 200 with HTML containingWelcome to TanStack Start.cargo build --releasecleancargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windowspasses(if user-facing) Added or updated a test under
test-files/or a#[test]in the affected crate(if CLI / stdlib / runtime API changed) Updated
docs/src/is not needed; this changes compiler/runtime compatibility for existing APIs(if touching a platform UI backend) Not applicable
Screenshots / output
n/a
Checklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit
New Features
Response/Requestbody handling and Web Streams integration.Bug Fixes
TextEncoder/TextDecoderusage in compiled code.super(), dynamic import, and property-access edge cases.