Migrate OSS dummy client examples to TypeScript#3606
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 80f04e80de
ℹ️ 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".
Code Review — PR #3606: Migrate OSS dummy client examples to TypeScriptThis is a well-structured, high-quality PR. The TypeScript migration is thorough, type discipline is strong (no 1. Pro registry silently narrows the new
|
| # | Severity | Area |
|---|---|---|
| 1 | Medium | Pro registry type cast — align stored type with OSS change or add explicit guard |
| 2 | Low | Missing unit tests for new throw paths in createReactOutput/ClientRenderer |
| 3 | Low | CHANGELOG/docs: note .component type narrowing required for TS consumers |
| 4 | Nit | Comment on why /x regex with union of paths is safe |
| 5 | Nit | Add .mts/.cts to add_generated_pack_to_server_bundle spec loop |
Items 4–5 are nits suitable for the deferred #3648 cleanup. Items 1–3 are worth addressing before merge.
| componentRegistry.set(name, { | ||
| name, | ||
| component, | ||
| component: component as ReactComponentOrRenderFunction, |
There was a problem hiding this comment.
The as ReactComponentOrRenderFunction cast silently discards the Record<string, unknown> arm of the new RegisteredComponentValue union at the type level. The OSS registry stores component typed as RegisteredComponentValue; the pro registry narrows it back to the function-only type at storage time.
If a plain-object server_render_js registration ever flows through the pro bundle, downstream runtime guards in createReactOutput / delegateToRenderer will still fire — but callers that read .component from this registry will see the wrong type and any function-call guard that depends on the stored type won't trigger.
Consider one of:
- Widening the
CallbackRegistrygeneric toRegisteredComponent<RegisteredComponentValue>(aligning with the OSS change), or - Keeping the narrowed type but adding an explicit pre-storage guard that throws/warns when
typeof component !== 'function', instead of casting.
| component: component as ReactComponentOrRenderFunction, | |
| component: component as ReactComponentOrRenderFunction, |
| } | ||
|
|
||
| if (typeof component !== 'function') { | ||
| throw new Error(`Registered render function "${name}" must be a function.`); |
There was a problem hiding this comment.
This new guard (and its sibling at line ~135 for the component path) has no dedicated test. A unit test asserting that registering a plain object and calling through the render-function path throws "must be a function" would document the contract. The corresponding guard in ClientRenderer.ts has the same gap.
| import_path_to_generated_server_bundle, | ||
| legacy_relative_import_path_to_generated_server_bundle | ||
| ) | ||
| generated_server_bundle_import_regex = / |
There was a problem hiding this comment.
The /x (extended/free-spacing) mode strips literal whitespace from the regex. Regexp.union with string arguments auto-escapes them (per Ruby docs), so the union alternatives are safe. However, the \s+ between import and the quote is also inside the /x block — this still works because \s is a character class escape, not a literal space.
Worth a brief inline comment that /x is intentional here (free-spacing for readability) and that the union alternatives contain no spaces, so stripping is harmless — saves future readers from having to rediscover this.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61e13cd3c
ℹ️ 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".
| }; | ||
|
|
||
| type ReactComponent = ComponentType<any> | string; | ||
| type ReactComponent = ComponentType<any> | ExoticComponent<any> | string; |
There was a problem hiding this comment.
Because ReactComponent now includes ExoticComponent, TypeScript render functions can now validly resolve to React.memo, React.forwardRef, or React.lazy component objects, but the createReactOutput promise branch still only calls createElement when typeof result === 'function'. In that async scenario the exotic component object is returned raw to ReactDOM/server rendering instead of being wrapped as an element, so it fails at runtime; please apply the same React object component handling used for synchronous registrations to promised render-function results.
Useful? React with 👍 / 👎.
* origin/main: Improve create-app smoke diagnostics (#3599) Suppress regression issues when only /posts_page: Pro regresses (#3668) Harden ShakaPerf artifact setup Polish RSC generator install follow-ups Harden renderer teardown polish fix(benchmark): harden target monitoring Docs: clarify rails new JavaScript skip flag (#3666) Tighten benchmark summary annotations Decouple Shakapacker config helpers from SystemChecker # Conflicts: # packages/react-on-rails/src/createReactOutput.ts
PR Review: Migrate OSS dummy client examples to TypeScriptOverviewThis PR accomplishes two complementary things:
StrengthsCore framework changes:
Ruby packs generator:
TypeScript migration quality:
Issues and Observations1.
2. Duplicate alternatives in In 3. The old 4. Relative doc links (informational) The SummaryThe code is well-written, well-tested, and the design decisions (generic default, extension resolution priority, import idempotency across formats) are sound. The Ruby spec coverage for the new resolution logic is particularly thorough. LGTM with the minor observations above. |
| // React.memo, React.forwardRef, React.lazy, and related component types are non-callable | ||
| // objects tagged with React's element-type marker. | ||
| const typeMarker = (value as { $$typeof?: unknown }).$$typeof; | ||
| return typeof typeMarker === 'symbol' || typeof typeMarker === 'number'; |
There was a problem hiding this comment.
This check accepts any object whose $$typeof is any symbol or number. The parallel guard in strictModeSupport.tsx:isObjectComponent is more precise — it validates against a Set of specific known React component type symbols:
const componentType = (component as ObjectComponent).$$typeof;
return componentType !== undefined && getReactObjectComponentTypes().has(componentType);The difference: a plain object with $$typeof: Symbol('custom') passes here but would be rejected there. In practice this is unlikely to cause problems (registered components are intentional), but the inconsistency could be surprising. Consider importing/reusing the same Set-based check, or at minimum add a comment noting that this is intentionally looser (duck-typing for any React-like marker vs. exhaustive known-symbol list).
Pro Node Renderer Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
| generated_server_bundle_import_pattern = Regexp.union( | ||
| relative_import_path_to_generated_server_bundle, | ||
| import_path_to_generated_server_bundle, | ||
| legacy_relative_import_path_to_generated_server_bundle | ||
| ) | ||
| generated_server_bundle_import_regex = / | ||
| import\s+['"] | ||
| #{generated_server_bundle_import_pattern} | ||
| ['"] | ||
| /x |
There was a problem hiding this comment.
The three-alternative union is correct, but the first two alternatives are identical when the source entrypoint is not .js (e.g. .ts, .tsx, .jsx). generated_server_bundle_import_path only strips the .js suffix when the source entrypoint is itself .js — for all other extensions it returns the same explicit .js path that relative_import_path already produced.
This is harmless (a duplicate in a Regexp.union is silently redundant), but could confuse a future maintainer who wonders why three alternatives are needed. Consider a brief comment:
# Three alternatives for idempotency across path formats:
# 1. relative_import_path_to_generated_server_bundle — current normalized path (always with .js)
# 2. import_path_to_generated_server_bundle — .js stripped for .js-source extensionless imports
# (same as #1 for non-.js sources; duplicate is harmless)
# 3. legacy "./" prefixed path from old generator output
generated_server_bundle_import_pattern = Regexp.union(
| "test:e2e:debug": "npx playwright test --config=e2e/playwright.config.js --debug", | ||
| "test:e2e:report": "npx playwright show-report e2e/playwright-report", | ||
| "build:test": "rm -rf public/webpack/test && pnpm run build:rescript && RAILS_ENV=test NODE_ENV=test bin/shakapacker", | ||
| "build:test": "rm -rf public/webpack/test && RAILS_ENV=test NODE_ENV=test bin/shakapacker-precompile-hook && SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true RAILS_ENV=test NODE_ENV=test bin/shakapacker", |
There was a problem hiding this comment.
The switch from pnpm run build:rescript && bin/shakapacker to bin/shakapacker-precompile-hook && ... bin/shakapacker is correct — the hook runs build:rescript before pack generation when a ReScript config is present, and SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true prevents it from running again as part of the main shakapacker call.
That said, the behavior is now implicit: if someone adds a new pre-compilation step and forgets to wire it into the hook, build:test will silently skip it. A short comment on this line would help contributors understand why the hook is used instead of an explicit build:rescript call:
"build:test": "rm -rf public/webpack/test && RAILS_ENV=test NODE_ENV=test bin/shakapacker-precompile-hook && SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true RAILS_ENV=test NODE_ENV=test bin/shakapacker",e.g. add an npm comment key or note in a nearby README that the hook runs build:rescript (and any future pre-compilation steps) before pack generation.
Core Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 1/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
Pro (shard 2/2) Benchmark Summary
▲/▼ non-zero change vs baseline · 0.0% exact/near-zero match · 🔴 significant regression · 🟢 significant improvement (tracked measures) · (n) = baseline |
…lement * origin/main: Migrate OSS dummy client examples to TypeScript (#3606) Docs: use async props pattern in metadata and render-function examples (#3672) Require PR numbers in squash merge titles (#3684) Improve create-app smoke diagnostics (#3599) Suppress regression issues when only /posts_page: Pro regresses (#3668) Harden ShakaPerf artifact setup Polish RSC generator install follow-ups Harden renderer teardown polish fix(benchmark): harden target monitoring Docs: clarify rails new JavaScript skip flag (#3666) Tighten benchmark summary annotations Decouple Shakapacker config helpers from SystemChecker Derive Pro RSC client refs from the RSC graph (#3556) Docs: follow up v15 anchor and Rails 5.2 flag # Conflicts: # react_on_rails/spec/dummy/client/app/startup/ManualRenderApp.jsx # react_on_rails/spec/dummy/client/app/startup/ReduxApp.client.jsx # react_on_rails/spec/dummy/client/app/startup/ReduxSharedStoreApp.client.jsx
…cessing-flow * origin/main: Add maintainer +ci commands for full-CI dispatch and waivers (#3687) Adopt renderer-function teardown in dummy apps + docs (#3578) (#3585) Migrate OSS dummy client examples to TypeScript (#3606) Docs: use async props pattern in metadata and render-function examples (#3672) Require PR numbers in squash merge titles (#3684)
## Summary - Document why the dummy Redux state keeps `railsContext` indexable while retaining the RailsContext fields. Fixes #3648. ## Codex Decision Log - **Non-blocking:** #3648 is a mixed bucket of optional PR #3606 review nits. - **Decision:** Implement only the bounded Redux state-indexing documentation note. - **Why:** It is a one-line explanation on the exact type shape under review and avoids broad generator, Redux Toolkit, path-regex, or shared startup refactors. - **Review later:** The remaining #3648 candidates should stay separate if maintainers want them. ## Test plan - `script/ci-changes-detector origin/main` - pass; source comments only, recommends lint. - `git diff --check origin/main..HEAD` - pass. - `pnpm run lint` - pass. - `pnpm start format.listDifferent` - pass. - `pnpm run type-check` from `react_on_rails/spec/dummy` - pass. - `bundle exec rubocop` - fails on pre-existing unrelated `react_on_rails/spike/3313_prism_gemfile_rewriter/*` offenses; this PR changes no Ruby files. ## Review gates - Manual diff review used. - `codex review --base origin/main` skipped because this is a trivial one-line source-comment change. - Claude review and `/simplify` skipped; no high-risk/full-ci/benchmark scope. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Comment-only change in a spec dummy file; no behavior, types, or production code affected. > > **Overview** > Adds an inline comment on **`ReduxAppState`** in the dummy app’s `reduxTypes.ts` explaining why **`railsContext`** is typed as `RailsContext & Record<string, unknown>`: **`combineReducers`** keys state by reducer name, and the intersection keeps normal RailsContext fields while still allowing that indexed lookup. > > No runtime or type-shape changes—documentation only for reviewers of the dummy Redux example (addresses #3648). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit dc4c4db. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
2 participants
Summary
tsc --noEmitcoverage for the OSS dummy client app and let Shakapacker choose SWC parser settings per source extension.server-bundle.js) and existing plain-objectserver_render_jsregistrations.registerStoreGeneratorsAPI in the migrated dummy bundle for shared-store registration.Resolves #1583.
Scope
This intentionally leaves legacy or fixture-only JavaScript in place:
BrokenApp.jsxremains an intentionally broken fixture.HelloWorldES5.jsxremains ES5/createReactClass legacy coverage.Test plan
pnpm -r run type-checkpnpm start format.listDifferentpnpm run lintcurrently fails outside PR Migrate OSS dummy client examples to TypeScript #3606 scope intest/shakaperf/rsc-foucafter mergingmain(shaka-sharedunresolved, with cascading unsafe-type errors).(cd react_on_rails && bundle exec rubocop)git diff --checkAdditional verification performed during the branch work:
bundle exec rspec spec/packs_generator_spec.rbfromreact_on_rails/spec/dummy.react_on_rails:generate_packsidempotency check forserver-bundle.tsgenerated import.pnpm run build:testpassed with the OSS bundle and generated packs./,/render_js,/server_side_hello_world,/server_side_redux_app.server-bundle.tswhile outputtingrsc-bundle.js.react_on_rails:generate_packs:pnpm exec knip --exclude binariesandpnpm exec knip --production --exclude binaries.lychee --config .lychee.tomlis blocked because installed lychee 0.24.2 rejects the repo config field used by the pinned CI lychee version.Review follow-up notes:
4258650b: registration debug logging now labels function source length vs object export-key counts, generatedimport/extensionssuppression is scoped to TS/JSX-family server-bundle source entrypoints, andbuild:testintentionally relies onbin/shakapacker-precompile-hookbecause the shared hook runsbuild:rescriptbefore generated pack creation when ReScript config is present.origin/maininto the branch and resolved theCHANGELOG.mdconflict only.client/app/types/generated-packs.d.tsfor*/generated/server-bundle-generated.jsrather than committing an ignored generated stub, so clean-checkout dummy type-checking accepts the generated import while the real generated pack stays uncommitted.mainURLs would 404 before this PR merges, and pinned SHA URLs go stale after the migration.PRRT_kwDOAnNnU86HM0BD,PRRT_kwDOAnNnU86HM0VO,PRRT_kwDOAnNnU86HM0Xc,PRRT_kwDOAnNnU86HRvIZ,PRRT_kwDOAnNnU86HRvOS, andPRRT_kwDOAnNnU86HM0dG.Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Codex follow-up notes