Repository navigation
Conversation
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8904 +/- ##
==========================================
+ Coverage 81.14% 81.19% +0.05%
==========================================
Files 456 456
Lines 63760 63742 -18
==========================================
+ Hits 51738 51756 +18
+ Misses 12022 11986 -36
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93962f9715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8904 |
cknitt
marked this pull request as ready for review
October 19, 2026 14:48
cknitt
added a commit
that referenced
this pull request
Oct 19, 2026
A flat `A$B$make` export now only clashes with another export. A binding
hidden by the interface may have the same JS name: the printer renames the
local and exports the alias under the flat name.
Example (rescript):
// Lib.resi exposes A.B.make and both, but not \"A$B$make"
module A = {
module B = {
let make = () => 1
}
}
let \"A$B$make" = () => Math.random() > 2. ? 0 : 2
let both = () => A.B.make() + \"A$B$make"()
After (js):
function A$B$make() { ... }
let A$B$make$1 = make;
export { A, both, A$B$make$1 as A$B$make }
Source: tests/tests/src/flat_make_export_hidden.mjs in this diff; the
previous commit rejected this file.
Review: Codex finding on #8904, lam_compile_main.ml:186.
Area: lambda tests
Output: changed tests/tests/src/flat_make_export_*.mjs only, for the new test
Formatting: unchanged
Formats: none
PPX: unchanged
Editor: unchanged
Tests: +tests/tests/src/flat_make_export_hidden.res
~tests/tests/src/flat_make_export_test.res
~tests/build_tests/super_errors/expected/flat_make_export_collision.res.expected
PR: #8904
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh
Signed-off-by: Christoph Knittel <ck@cca.io>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cknitt
added a commit
that referenced
this pull request
Oct 19, 2026
The paths are a field of `Translmod.implementation`, built from the final
signature by `transl_implementation`, and `Lam_compile_main.compile` takes
the whole record. Each step of a path is a `{pos; name}` record instead of
an `(int * string)` pair. Consumer reads and dynamic imports share
`Translmod.flat_make_name`, and the aliases are plain `(ident, value)`
pairs.
Review: fhammerschmidt on #8904, lam_compile_main.mli:33, asked for a
labeled argument or a named type instead of `(int * string) list list`.
Area: lambda codegen playground tests
Output: unchanged, node scripts/test.js -all rebuilds every test project
with no generated file changing
Formatting: unchanged
Formats: none
PPX: unchanged
Editor: unchanged
Tests: none, because the change is a refactor covered by the existing
flat_make_export and nested_component tests
PR: #8904
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh
Signed-off-by: Christoph Knittel <ck@cca.io>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Member
|
Rechecked The awaited-namespace and server re-export ( |
This was referenced Oct 20, 2026
Every `make` in a nested module of a file's final signature is also exported as `A$B$make`, and other modules read it through that name. React Server Components only pass a client module's top-level exports to the client, so `<Sidebar.Provider />` no longer reads `.make` from a client reference. Both sides decide from the signature alone, which replaces `@res.hoistedFunction` and its `.cmj` table (#8402); see #8901. Example (rescript): // Sidebar.res module Provider = { @react.component let make = (~children) => children } // Page.res let page = <Sidebar.Provider> {React.null} </Sidebar.Provider> Before (js): // Page.res.js JsxRuntime.jsx(Sidebar.Provider.make, {children: null}) After (js): // Sidebar.res.js let Provider$make = Sidebar$Provider; export { Provider, Provider$make } // Page.res.js JsxRuntime.jsx(Sidebar.Provider$make, {children: null}) Source: After from tests/build_tests/react_ppx/src/nested_component_*.res.js in this diff. Before is master's fallback for a nested read without a `.cmj` hoisting entry. Breaking: `@res.hoistedFunction` is gone; it now reports the unused-attribute warning 101. Nested functions other than `make` are no longer exported flat. Consumers read `Lib.A$make` instead of `Lib.A.make`, so test spies or mocks that replace `make` on the module object no longer intercept those reads. Review: Why can't a `.resi` edit leave consumers stale, as in #8886? The flat export set is a function of the signature, so it changes exactly when the `.cmi` does, and both build systems already rebuild dependents then. Changing only `Sidebar.res` behind an unchanged `.resi` (a module alias, an external, a functor application, `include`) kept the `.cmi` byte-identical and every flat export in a scratch build. Review: Do the aliases defeat tree-shaking? When the module field holds a top-level binding, the alias is `let A$B$make = make`. Only fields without one (a wrapped value such as `Some(make)`, a functor result, an awaited module) fall back to a property read. Review: What about name clashes? A flat name already used by a top-level binding is a compile error, as it was for `@res.hoistedFunction`. Area: lambda codegen frontend typechecker playground tests Output: changed programs with nested `make` values gain flat exports and read them from other modules; 26 committed outputs change, all additive apart from the rewritten reads, and 3 go with the removed tests Formatting: unchanged Formats: cmi PPX: unchanged Editor: unchanged Errors: none, because the new clash error is an inline raise_errorf, outside the catalog's scope Tests: +tests/build_tests/react_ppx/src/nested_component_producer.res +tests/build_tests/react_ppx/src/nested_component_consumer.res +tests/build_tests/super_errors/fixtures/flat_make_export_collision.res ~tests/tests/src/flat_make_export*.res (from hoisted_function_attr*.res) ~tests/tests/src/import2.res ~tests/build_tests/super_errors/fixtures/warning_101_bs_unused_attribute.res -tests/build_tests/super_errors/fixtures/hoisted_function_*.res Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh Signed-off-by: Christoph Knittel <ck@cca.io>
A flat `A$B$make` export now only clashes with another export. A binding
hidden by the interface may have the same JS name: the printer renames the
local and exports the alias under the flat name.
Example (rescript):
// Lib.resi exposes A.B.make and both, but not \"A$B$make"
module A = {
module B = {
let make = () => 1
}
}
let \"A$B$make" = () => Math.random() > 2. ? 0 : 2
let both = () => A.B.make() + \"A$B$make"()
After (js):
function A$B$make() { ... }
let A$B$make$1 = make;
export { A, both, A$B$make$1 as A$B$make }
Source: tests/tests/src/flat_make_export_hidden.mjs in this diff; the
previous commit rejected this file.
Review: Codex finding on #8904, lam_compile_main.ml:186.
Area: lambda tests
Output: changed tests/tests/src/flat_make_export_*.mjs only, for the new test
Formatting: unchanged
Formats: none
PPX: unchanged
Editor: unchanged
Tests: +tests/tests/src/flat_make_export_hidden.res
~tests/tests/src/flat_make_export_test.res
~tests/build_tests/super_errors/expected/flat_make_export_collision.res.expected
PR: #8904
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh
Signed-off-by: Christoph Knittel <ck@cca.io>
The paths are a field of `Translmod.implementation`, built from the final
signature by `transl_implementation`, and `Lam_compile_main.compile` takes
the whole record. Each step of a path is a `{pos; name}` record instead of
an `(int * string)` pair. Consumer reads and dynamic imports share
`Translmod.flat_make_name`, and the aliases are plain `(ident, value)`
pairs.
Review: fhammerschmidt on #8904, lam_compile_main.mli:33, asked for a
labeled argument or a named type instead of `(int * string) list list`.
Area: lambda codegen playground tests
Output: unchanged, node scripts/test.js -all rebuilds every test project
with no generated file changing
Formatting: unchanged
Formats: none
PPX: unchanged
Editor: unchanged
Tests: none, because the change is a refactor covered by the existing
flat_make_export and nested_component tests
PR: #8904
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh
Signed-off-by: Christoph Knittel <ck@cca.io>
cknitt
force-pushed
the
jsx-flat-exports
branch
from
October 21, 2026 12:01
6e835ba to
fa472db
Compare
A flat export of a module that re-exports another file's module, as in
`module Provider = Sidebar.Provider`, now aliases that file's flat export
instead of reading `make` from its module object. Reads below a local bound
to an awaited module, `module D = await Sidebar`, use the flat names too. Both
cases read `make` from a React Server Components client reference before.
Example (rescript):
// Reexport.res, with a .resi exposing Provider.make
module Provider = Sidebar.Provider
// Page.res
let load = async () => {
module D = await Sidebar
<D.Provider> {React.null} </D.Provider>
}
Before (js):
// Reexport.res.js
let Provider$make = Provider.make;
// Page.res.js
let D = await import("./Sidebar.res.js");
return JsxRuntime.jsx(D.Provider.make, {children: null});
After (js):
// Reexport.res.js
let Provider$make = Sidebar.Provider$make;
// Page.res.js
let D = await import("./Sidebar.res.js");
return JsxRuntime.jsx(D.Provider$make, {children: null});
Source: bsc of the previous commit and of this one on a scratch copy of
the two files; the same shapes are in
tests/build_tests/react_ppx/src/nested_component_{reexport,consumer}.res.js.
Review: fhammerschmidt's Next.js end-to-end run on #8904 failed
prerendering for exactly these two cases (rescript-lang/end-to-end#16).
Review: `module P = await Sidebar.Group` still reads `P.Provider.make`: `P`
is Sidebar's `Group` export, so its flat names are out of reach without
changing what `P` holds.
Area: lambda codegen tests
Output: changed tests/tests/src/Import.mjs, where an alias now reads the
awaited module directly (`O.make` instead of `N1.O.make`), and the new
test outputs
Formatting: unchanged
Formats: none
PPX: unchanged
Editor: unchanged
Tests: +tests/build_tests/react_ppx/src/nested_component_reexport.res
~tests/build_tests/react_ppx/src/nested_component_consumer.res
+tests/tests/src/flat_make_export_reexport.res
~tests/tests/src/flat_make_export_test.res
PR: #8904
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh
Signed-off-by: Christoph Knittel <ck@cca.io>
Member
Author
|
@fhammerschmidt Thanks for the end-to-end run! Both failing cases are addressed in 63a47ff (rebased on master first):
Not covered: Could you rerun the Next.js cases against the new pkg.pr.new build? |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
tsnobip
approved these changes
Oct 21, 2026
tsnobip
left a comment
tsnobip
left a comment
Member
There was a problem hiding this comment.
the fix works on my project, I like the solution!
Member
|
Also works in our new end-to-end test NExtJS-Repo! |
Member
|
The extended Next.js checks are now merged in rescript-lang/end-to-end#16. The run against To rerun manually:
Or use the CLI: gh workflow run validate-nextjs-rsc.yml \
--repo rescript-lang/end-to-end --ref main \
-f compiler=8904 -f scenario=allRun after the compiler's pkg.pr.new publication completes. You can replace |
This branch has not been deployed
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
3 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.
Closes #8901. Alternative to #8886 and #8293; removes
@res.hoistedFunction(#8402).Problem
React Server Components only pass a client module's top-level exports to the client. Nested components such as
<Sidebar.Provider />compiled toSidebar.Provider.make, a read through the module object, which fails on a client reference.#8402/#8886 add flat exports depending on the producer's implementation, and record them in the
.cmjonly. With a.resi, an implementation-only edit can drop an export while the.cmistays byte-identical, so dependents are not rebuilt and crash at runtime (see the review of #8886).Change
Every
makein a nested module of a file's final signature (the.resi's, if there is one) is also exported asA$B$make, and reads ofLib.A.B.makefrom other modules compile toLib.A$B$make..cmjtable, no marker attribute, no JSX transform changes. The export set changes exactly when the.cmidoes.let A$B$make = makethat bundlers drop when unused. It falls back to a property read only when the field holds no plain binding (Some(make), a functor result, an awaited module).module rec, relative externals,include, module aliases and functor applications all get flat exports. Components hidden by the.residon't.import(Lib.A.B.make)imports the flat export, and reads belowmodule D = await Libuse the flat names (D.A$B$make).module Provider = Sidebar.Provider) re-exports that file's flat export (let Provider$make = Sidebar.Provider$make).@res.hoistedFunction, its.cmjtable and lookups are removed; a leftover use reports warning 101. The.cmimagic number is bumped.Breaking
@res.hoistedFunctionis gone (it shipped only in v13.0.0-alpha.6). Nested functions other thanmakeare no longer exported flat.Lib.A$makeinstead ofLib.A.make, so test spies or mocks that replacemakeon the module object no longer intercept those reads.Bundle size
Bundling a producer with 200 nested components through esbuild, Rollup, Rolldown and webpack gives the same or slightly smaller bundles than master; unused flat exports are dropped.
Tests
tests/build_tests/react_ppx/src/nested_component_{producer,consumer}.res(with a.resi, namespaced): every nested JSX tag reads a flat export; the hidden component has none.tests/tests/src/flat_make_export*.res(replacinghoisted_function_attr*): nested, deep, recursive, coerced, rebound, functor-applied, included and aliasedmakevalues across modules.tests/build_tests/react_ppx/src/nested_component_reexport.res(with a.resi) and the awaited reads innested_component_consumer.res: re-exports and awaited namespaces go through flat exports.tests/tests/src/flat_make_export_reexport.resand the awaited case inflat_make_export_test.res: the same at runtime.tests/tests/src/import2.res: dynamic import of a nestedmake.super_errors/flat_make_export_collision.res, andwarning_101_bs_unused_attribute.resfor a leftover@res.hoistedFunction.The other changed JS snapshots only gain flat exports or read them.
🤖 Generated with Claude Code
https://claude.ai/code/session_013GUg1NbeoJMi6t6V89yBWh