Repository navigation
Conversation
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>
|
@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.04% 81.07% +0.02%
==========================================
Files 455 455
Lines 63508 63460 -48
==========================================
- Hits 51469 51449 -20
+ Misses 12039 12011 -28
🚀 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".
| if Set_string.mem js_name occupied_names then | ||
| Location.raise_errorf ~loc | ||
| "Cannot export `%s` as `%s` because that name is already used \ | ||
| by a top-level binding." | ||
| (String.concat "." names) name; |
There was a problem hiding this comment.
When a file has a .resi, this rejects a private implementation binding whose JS name happens to equal a generated flat export. For example, an implementation can expose A.B.make while retaining a used but signature-hidden let "A$B$make" = ...; because occupied_names contains every surviving Lambda group rather than only export_ids, compilation now reports a collision even though the private local can be renamed and only the generated alias needs the public A$B$make export name. Limit this collision check to bindings that are actually exported so previously valid implementations remain compilable.
Useful? React with 👍 / 👎.
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@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 |
This branch has not been deployed
1 participant
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.@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/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