Repository navigation
Prepare Relay conversions once and harden scalar handling - #674
Conversation
zth
changed the title
|
@codex review Please review the current head against the base, focusing on conversion correctness, compiler/runtime compatibility, generated-code and release-module behavior, and gaps in the tests. The paired compiler change is zth/relay#40 and is being reviewed separately. Please report actionable issues with severity and a reproducing scenario. |
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. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
|
@codex review the full current PR at 64c0934. The prior runtime review found no major issues. The compiler review led to a plural @catch fix in zth/relay#40 at 6247069896de, plus mounted regressions here for ordinary/caught plural scalars, typed result wrappers, updates, mixed errors/successes, and raw-store preservation. Please check the new changes and any remaining major correctness/performance/packaging issues across the integration. All 250 main/persisted tests, 34 compiler library tests, 18 mutation checks and size budgets pass. |
|
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". |
|
@codex review the full current PR at 65d953f. The previous review found no major issues. Since then the compiler review exposed nullable and abstract plural @catch shapes: fixed in zth/relay#40 at 19bfb3d85aeb, with additional typed and mounted regressions here covering nullable success values, unions/interfaces, mixed success/error results, updates and raw-store preservation. Runtime unchanged. All 250 main/persisted tests, 35 compiler library tests and bundle-size budgets pass; runtime coverage remains 100%. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
zth
changed the title
1 participant
Prepare conversion functions once per generated module instead of interpreting paths on every Relay snapshot. Preserve legacy generated-artifact support while fixing custom-scalar ordering, nested lists, path collisions, recursive inputs, union conversion order, and plural
@catchpayloads.Dependency: merge zth/relay#40 first, retaining the pinned compiler commit in history; then merge this PR. Release compiler and runtime together. New artifacts require the new runtime; existing artifacts remain supported. If the compiler PR is squash-merged, update this submodule pin to its merged commit before merging here.
Validation: 263 main/persisted JS tests, including 157 utility tests; 35 compiler library tests; 20/20 targeted mutations detected; 100% runtime statement/branch/function/line coverage. Scalar tests exercise 960 ordering combinations twice, plus opposite selection orders through mounted hooks. Both compiler fixture projects regenerate deterministically and pass
--validate. Codex reviewed the implementation with no remaining major findings. All parent CI checks pass on the final cleanup commit, including integration, bindings, CLI, and Linux/macOS/Windows PPX builds.Performance and size: recorded conversion-only benchmarks versus master show roughly 13–14× for small fragments and 41–45× for 100-row connections; the Date-heavy scalar case is 5–7% slower. Results are workload/engine dependent, not app-wide latency claims. Across 129 fixed artifacts, combined gzip is 1,254 bytes smaller; the shared runtime including legacy support is 1,265 bytes larger. These separately measured components are approximately neutral together, not a whole-app bundle measurement.
See
packages/rescript-relay/benchmarks/{RESULTS,SIZE}.mdanddocs/conversion-contract.mdfor evidence, reproduction and limits. The rejected ReScript prototype was removed from the merge diff and remains accessible through a history link in the benchmark guide.