test attempt#583
Open
ValdemarGr wants to merge 4 commits into
Open
Conversation
drewwestphal
added a commit
to drewwestphal/rescript-relay-custom-scalar-null-2026
that referenced
this pull request
Jun 1, 2026
ValdemarGr's stale PR zth#583 added a schema input shape, two custom scalar modules, and a relay-config registration to reproduce issue zth#582 (encoding doesn't always apply scalar encoders). This commit lifts those fixtures in preparation for a full integration test for zth#582 and broader regression coverage in this PR. ValdemarGr's ObjectScalar serializers and parsers also log to the console, which the harness in a later commit uses to assert which converters actually ran for a given operation. The Datetime and Number serializers in TestsUtils gain similar console.log lines (matching the existing pattern in IntString) so a follow-up zth#407 regression test can verify that a non-array custom scalar variable still has its serializer run when it follows an array-backed scalar variable in the same operation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zth
pushed a commit
that referenced
this pull request
Jun 1, 2026
* Add ObjectScalar custom-scalar fixtures from #583 ValdemarGr's stale PR #583 added a schema input shape, two custom scalar modules, and a relay-config registration to reproduce issue #582 (encoding doesn't always apply scalar encoders). This commit lifts those fixtures in preparation for a full integration test for #582 and broader regression coverage in this PR. ValdemarGr's ObjectScalar serializers and parsers also log to the console, which the harness in a later commit uses to assert which converters actually ran for a given operation. The Datetime and Number serializers in TestsUtils gain similar console.log lines (matching the existing pattern in IntString) so a follow-up #407 regression test can verify that a non-array custom scalar variable still has its serializer run when it follows an array-backed scalar variable in the same operation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add integration test for #582 (encoding) — ca followed by c in mutation input Reproduces the bug ValdemarGr reported in issue #582 using SerializeMultipleCustomScalars: the input has `os1s: [ObjectScalar1!]!` followed by `os2: ObjectScalar2`. On master the early-return in the `ca` branch of traverse (utils.js:109-113) skips the `c` instruction for `os2`, so ObjectScalar2.serialize is never called. The harness asserts that both serializers' log lines fire. The fix for #582 lands later in this PR; on this commit the test fails for the right reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add integration test for #582 (decoding) — c following ca in response Read-side analog of the bug ValdemarGr's #582 reports on the encoding side: a `c` custom-scalar field (createdAt: Datetime!) follows a `ca` custom-scalar array (intStrings) in a query selection. On master the early-return in traverse (utils.js:109-113) leaks the raw wire string through Object.assign untouched, so createdAt never reaches Datetime.parse. The harness renders `createdAt->Date.getTime->Float.toString` — Date.getTime on a raw string throws, so the assertion never resolves on master. Fails on this commit, passes once the fix lands later in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add integration test for #407 — c-array sibling-skip regression The `c && Array.isArray` branch in traverse (utils.js:134-138) was added by PR #434 to close issue #407 ("Array scalars are not well behaved"). It correctly handles single custom scalars whose ReScript representation is an array — but it terminates with `return` instead of `continue`, so sibling fields processed after the array-backed scalar in the same selection set have their converters skipped. This test runs a query with two variables: `$aaNumber: Number!` (custom scalar whose underlying type is array<int>) and `$beforeDate: Datetime`. relay-compiler sorts variables alphabetically in the generated record, so the `aa` prefix forces the array-backed scalar to be iterated first. On master only Number.serialize runs; Datetime.serialize is silently skipped. The fix later in this PR extends #434's intent so trailing fields are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add integration test for #631 — nullable scalars following a custom-scalar array A query selects `intStrings` (a `[IntString!]`, `ca`) followed by nullable plain scalars on the same parent. On master the early- return in the `ca` branch of traverse (utils.js:109-113) leaves those siblings copied through Object.assign untouched, so their raw wire `null` reaches the caller without going through the null coercion at the top of the per-key loop. ReScript then reads the leaked `null` as `Some(null)` instead of `None` (pattern matches take the wrong branch). The harness uses Type.classify (via Obj.magic + Null.toOption) on the `private` payload so the rendered DOM literally reads `private=Some(null)` on master — the smoking-gun assertion. The fix later in this PR turns the `return` into `continue` and the DOM reads `private=None`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add c/ca behavioral matrix to utils-tests.js Maps the converter-instruction surface area covered by this PR. Twenty-five direct calls into `traverser` covering each `c` and `ca` wire-payload shape in both directions (read with nullableValue=undefined, write with nullableValue=null), plus explicit regression cells for the three open/closed issues the fix addresses: - #631 — `ca` followed by null sibling - #582 — `ca` followed by another `c` - #407 — single `c` whose underlying type is an array Plus sequencing cells that document order-dependence (two `ca`s in a row, `ca` followed by a nested object with its own `c`, and a null sibling BEFORE `ca` as a positive control showing the bug only fires when `ca` precedes other fields). Two cells under "ca — custom scalar array (read/write)" explicitly lock in the CURRENT BEHAVIOR for `[T]!` and `[T]` (nullable-element) arrays. relay-compiler emits the same `ca` instruction for all four GraphQL array nullability variants, and the existing fast-path calls `converter(null)` on null elements. That is almost certainly a separate latent bug — out of scope for this PR. The cells make it explicit so a future fix is a deliberate change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix traverse exiting early after custom-scalar conversion The two custom-scalar branches in `traverse` (utils.js) exit the per-key loop with `return newObj` instead of moving on with `continue`: - line 112 — `shouldConvertCustomFieldArray && Array.isArray(...)` (`ca` instruction, e.g. `[IntString!]`). - line 137 — `shouldConvertCustomField && Array.isArray(...)` (`c` instruction whose runtime value is an array, e.g. a custom scalar like `Number = array<int>`. This branch was added by PR #434 to close issue #407.) In both cases the shallow `Object.assign({}, currentObj)` done by `getNewObj` has already copied subsequent keys to `newObj`, so their raw values reach the caller without going through the null-coercion at the top of the loop (lines 76-79) or the rest of the per-key logic. The visible symptoms surfaced in three open issues: - #631 (this PR): ReScript reads leaked response-side `null` as `Some(null)` instead of `None`; pattern matches take the wrong branch. - #582 (ValdemarGr): the next variable's `c` instruction never runs, so the raw ReScript value ships on the wire instead of the serialized form. - #407 trailing-field regression: same as above but for a single array-backed scalar followed by another converted field — silently leaked. The leading comment on each branch ("Ensures we don't accidentally move into the array when we're not supposed to") matches the intent of skipping the remaining branches for the current key, i.e. `continue`. `return` exits the whole function and was unintended. Closes #582. Extends #434's fix for #407. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
No description provided.