Feature/recursive transformations and register serializable class#351
Feature/recursive transformations and register serializable class#351ZiadTaha62 wants to merge 10 commits intoflightcontrolhq:mainfrom
Conversation
- New SerializableClassRegistry - registerSerializableClass method - New 'serializable-class' transformer rule - Update plainer.ts for deep traversal - Add 4 new tests
fix: fixed issue where serializable class return non deep value
added: recursive option in custom transformation
…ity in recursive paths
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/plainer.ts
Outdated
| @@ -210,84 +201,110 @@ export const walker = ( | |||
| } | |||
There was a problem hiding this comment.
dedupe: true and shared object references in recursive transformations
When SuperJSON is created with { dedupe: true } and a serializable class's toSuperJSON() returns an object reference that was already serialized at another path in the same data structure, the current code will:
- Skip
addIdentity(transformedValue, path, identities)becauseisTransformation = true - Hit the
seenObjects.get(object)cache and return{ transformedValue: null }(dedupe path) - No referential equality annotation is generated (since
identitieshas only the original path)
The result is that the serialized JSON has null at the container's position with no annotation to restore it. On deserialization, deserializer.deserialize(null) is called instead of the correct value.
Example:
const sj = new SuperJSON({ dedupe: true });
const sharedObj = { x: 1 };
class Container { toSuperJSON() { return sharedObj; } }
sj.registerSerializableClass(Container);
const data = { direct: sharedObj, wrapped: new Container() };
sj.serialize(data); // wrapped.json will be null with no referential equality entryThe fix would be to call addIdentity even for isTransformation = true paths, but only after checking seenObjects, or alternatively, return seen (non-null) instead of null in this case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plainer.ts
Line: 191-201
Comment:
**Data loss with `dedupe: true` and shared object references in recursive transformations**
When `SuperJSON` is created with `{ dedupe: true }` and a serializable class's `toSuperJSON()` returns an object reference that was already serialized at another path in the same data structure, the current code will:
1. Skip `addIdentity(transformedValue, path, identities)` because `isTransformation = true`
2. Hit the `seenObjects.get(object)` cache and return `{ transformedValue: null }` (dedupe path)
3. No referential equality annotation is generated (since `identities` has only the original path)
The result is that the serialized JSON has `null` at the container's position with no annotation to restore it. On deserialization, `deserializer.deserialize(null)` is called instead of the correct value.
Example:
```typescript
const sj = new SuperJSON({ dedupe: true });
const sharedObj = { x: 1 };
class Container { toSuperJSON() { return sharedObj; } }
sj.registerSerializableClass(Container);
const data = { direct: sharedObj, wrapped: new Container() };
sj.serialize(data); // wrapped.json will be null with no referential equality entry
```
The fix would be to call `addIdentity` even for `isTransformation = true` paths, but only after checking `seenObjects`, or alternatively, return `seen` (non-null) instead of `null` in this case.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
I ran the earlier test:
test('Data loss with dedupe: true and shared object references in recursive transformations', () => {
const sj = new SuperJSON({ dedupe: true });
const sharedObj = { x: 1 };
class Container {
static fromSuperJSON() {
return new Container();
}
toSuperJSON() {
return sharedObj;
}
}
sj.registerSerializableClass(Container);
const data = { direct: sharedObj, wrapped: new Container() };
const serialized = sj.serialize(data);
const deserialized = sj.deserialize(serialized);
console.debug('Data: ');
console.dir(data, { depth: null });
console.debug('Serialized: ');
console.dir(serialized, { depth: null });
console.debug('Deserialized: ');
console.dir(deserialized, { depth: null });
expect(deserialized).toEqual(data);
});The test passed and these were the logs:
stdout | src/index.test.ts > Data loss with dedupe: true and shared object references in recursive transformations
Data:
{ direct: { x: 1 }, wrapped: Container {} }
Serialized:
{
json: { direct: { x: 1 }, wrapped: null },
meta: {
values: { wrapped: [ [ 'serializable-class', 'Container' ] ] },
v: 1
}
}
Deserialized:
{ direct: { x: 1 }, wrapped: Container {} }I see the issue now, even if the example test succeeded, the meta object of serialized is missing referentialEqualities, which will cause bugs as null will be passed to static fromSuperJSON
I will see what can i do to avoid this
src/custom-transformer-registry.ts
Outdated
| } | ||
|
|
||
| export class CustomTransformerRegistry { | ||
| private transfomers: Record<string, CustomTransfomer<any, any>> = {}; | ||
|
|
||
| register<I, O extends JSONValue>(transformer: CustomTransfomer<I, O>) { | ||
| register<I, O extends SuperJSONValue>(transformer: CustomTransfomer<I, O>) { | ||
| this.transfomers[transformer.name] = transformer; |
There was a problem hiding this comment.
Changing O extends JSONValue to O extends SuperJSONValue is intended to support recursive: true transformers (where the serialized value itself gets further processed by SuperJSON). However, this type change also allows non-recursive transformers to declare a non-JSON output type (like Date, Set, Map, etc.) without recursive: true.
When a non-recursive custom transformer returns a non-JSON value (e.g., Date), the walker returns early:
if (!isDeep) {
return { transformedValue: value, annotations: [type] };
}The value (a Date instance) is stored as transformedValue. When JSON.stringify is called it serializes the Date as an ISO string via JavaScript's default toJSON(). But on deserialization, transformer.deserialize(isoString) receives a string — not the Date object the serializer produced. This is a silently incorrect round-trip.
Consider using a discriminated union to restrict the O type based on the recursive flag:
export interface CustomTransfomer<I, O extends JSONValue> {
name: string;
isApplicable: (v: any) => v is I;
serialize: (v: I) => O;
deserialize: (v: O) => I;
recursive?: false;
}
export interface RecursiveCustomTransfomer<I, O extends SuperJSONValue> {
name: string;
isApplicable: (v: any) => v is I;
serialize: (v: I) => O;
deserialize: (v: O) => I;
recursive: true;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/custom-transformer-registry.ts
Line: 10-16
Comment:
**Overly permissive type constraint breaks non-recursive custom transformers**
Changing `O extends JSONValue` to `O extends SuperJSONValue` is intended to support `recursive: true` transformers (where the serialized value itself gets further processed by SuperJSON). However, this type change also allows non-recursive transformers to declare a non-JSON output type (like `Date`, `Set`, `Map`, etc.) without `recursive: true`.
When a non-recursive custom transformer returns a non-JSON value (e.g., `Date`), the walker returns early:
```typescript
if (!isDeep) {
return { transformedValue: value, annotations: [type] };
}
```
The `value` (a `Date` instance) is stored as `transformedValue`. When `JSON.stringify` is called it serializes the `Date` as an ISO string via JavaScript's default `toJSON()`. But on deserialization, `transformer.deserialize(isoString)` receives a **string** — not the `Date` object the serializer produced. This is a silently incorrect round-trip.
Consider using a discriminated union to restrict the `O` type based on the `recursive` flag:
```typescript
export interface CustomTransfomer<I, O extends JSONValue> {
name: string;
isApplicable: (v: any) => v is I;
serialize: (v: I) => O;
deserialize: (v: O) => I;
recursive?: false;
}
export interface RecursiveCustomTransfomer<I, O extends SuperJSONValue> {
name: string;
isApplicable: (v: any) => v is I;
serialize: (v: I) => O;
deserialize: (v: O) => I;
recursive: true;
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
The updated type from JSONValue to SuperJSONValue was done to support recursive custom transformations if needed as i found it useful and enable complex custom transformations without need to call nested serialize to every class or non JSON value
However using discriminated union for it will give much better type safety indeed
src/plainer.ts
Outdated
| objectsInThisPath: any[] = [], | ||
| seenObjects = new Map<unknown, Result>() | ||
| seenObjects = new Map<unknown, Result>(), | ||
| isTransformation: boolean = false // Prevent overwrite of original object in identities map in recursive transformtion |
There was a problem hiding this comment.
The isTransformation parameter is an internal implementation detail — it controls whether addIdentity is called during a recursive deep-transform pass. Since walker is exported from plainer.ts and called from index.ts, this flag is part of the de-facto public API. External callers that pass isTransformation: true would bypass the identity tracking and produce incorrect referential-equality annotations.
Consider wrapping the recursive call logic inside a private helper (e.g., walkerInternal) and keeping only the public-facing signature clean:
export const walker = (
object: any,
identities: Map<any, any[][]>,
superJson: SuperJSON,
dedupe: boolean,
path: any[] = [],
objectsInThisPath: any[] = [],
seenObjects = new Map<unknown, Result>()
): Result => walkerInternal(object, identities, superJson, dedupe, path, objectsInThisPath, seenObjects, false);Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plainer.ts
Line: 186
Comment:
**Internal flag exposed in public function signature**
The `isTransformation` parameter is an internal implementation detail — it controls whether `addIdentity` is called during a recursive deep-transform pass. Since `walker` is exported from `plainer.ts` and called from `index.ts`, this flag is part of the de-facto public API. External callers that pass `isTransformation: true` would bypass the identity tracking and produce incorrect referential-equality annotations.
Consider wrapping the recursive call logic inside a private helper (e.g., `walkerInternal`) and keeping only the public-facing signature clean:
```typescript
export const walker = (
object: any,
identities: Map<any, any[][]>,
superJson: SuperJSON,
dedupe: boolean,
path: any[] = [],
objectsInThisPath: any[] = [],
seenObjects = new Map<unknown, Result>()
): Result => walkerInternal(object, identities, superJson, dedupe, path, objectsInThisPath, seenObjects, false);
```
How can I resolve this? If you propose a fix, please make it concise.feat: removed serializable class registry to focus on one thing a time
| if (children) { | ||
| forEach(children, (child, key) => { | ||
| traverse(child, walker, version, [ | ||
| ...origin, | ||
| ...parsePath(key, legacyPaths), | ||
| ]); | ||
| }); | ||
| if (isArray(children)) { | ||
| traverse(children, walker, legacyPaths, origin); | ||
| } else { | ||
| traverseObject(children, walker, legacyPaths, origin, strOrigin); | ||
| } | ||
| } |
There was a problem hiding this comment.
strOrigin not propagated for recursive annotation children — causes incorrect deserialization
When an InnerNode's children is itself a Tree<T> (the case for recursive custom transformers, where the annotation looks like [['custom','Box'], ['Date']]), traverse calls itself without forwarding strOrigin. This means the walker callback is invoked with strPath = undefined for the inner annotation.
In setValueAnnotationsFn, the guard that prevents re-applying annotations to duplicate paths relies on strPath:
if (strPath && duplicate.has(strPath)) return;Because strPath is undefined, this guard never fires for inner annotations of recursive transformers.
Concrete failure (dedupe=false, same recursive transformer instance at two paths):
const box = new Box(new Date());
const data = { a: box, b: box }; // same instance- After serialization, both
aandbget annotation[['custom','Box'],['Date']]. - After the representative path
'a'is fully transformed (Box → Date → value set; ref-eq setsb = a), the walker continues to process'b''s annotation tree. - The inner
'Date'annotation fires withstrPath = undefined, soduplicate.has('b')is never checked. json.bis already aBoxinstance at this point;new Date(Box instance)producesInvalid Date.
Fix: pass strOrigin through the array branch, just as the object branch does:
| if (children) { | |
| forEach(children, (child, key) => { | |
| traverse(child, walker, version, [ | |
| ...origin, | |
| ...parsePath(key, legacyPaths), | |
| ]); | |
| }); | |
| if (isArray(children)) { | |
| traverse(children, walker, legacyPaths, origin); | |
| } else { | |
| traverseObject(children, walker, legacyPaths, origin, strOrigin); | |
| } | |
| } | |
| if (isArray(children)) { | |
| traverse(children, walker, legacyPaths, origin, strOrigin); | |
| } else { | |
| traverseObject(children, walker, legacyPaths, origin, strOrigin); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plainer.ts
Line: 59-65
Comment:
**`strOrigin` not propagated for recursive annotation children — causes incorrect deserialization**
When an `InnerNode`'s `children` is itself a `Tree<T>` (the case for recursive custom transformers, where the annotation looks like `[['custom','Box'], ['Date']]`), `traverse` calls itself without forwarding `strOrigin`. This means the `walker` callback is invoked with `strPath = undefined` for the inner annotation.
In `setValueAnnotationsFn`, the guard that prevents re-applying annotations to *duplicate* paths relies on `strPath`:
```ts
if (strPath && duplicate.has(strPath)) return;
```
Because `strPath` is `undefined`, this guard never fires for inner annotations of recursive transformers.
**Concrete failure** (dedupe=false, same recursive transformer instance at two paths):
```ts
const box = new Box(new Date());
const data = { a: box, b: box }; // same instance
```
- After serialization, both `a` and `b` get annotation `[['custom','Box'],['Date']]`.
- After the representative path `'a'` is fully transformed (Box → Date → value set; ref-eq sets `b = a`), the walker continues to process `'b'`'s annotation tree.
- The inner `'Date'` annotation fires with `strPath = undefined`, so `duplicate.has('b')` is never checked.
- `json.b` is already a `Box` instance at this point; `new Date(Box instance)` produces `Invalid Date`.
Fix: pass `strOrigin` through the array branch, just as the object branch does:
```suggestion
if (isArray(children)) {
traverse(children, walker, legacyPaths, origin, strOrigin);
} else {
traverseObject(children, walker, legacyPaths, origin, strOrigin);
}
```
How can I resolve this? If you propose a fix, please make it concise.
| function compositeTransformation<I, O, A extends CompositeTypeAnnotation>( | ||
| isApplicable: (v: any, superJson: SuperJSON) => v is I, | ||
| annotation: (v: I, superJson: SuperJSON) => A, | ||
| transform: (v: I, superJson: SuperJSON) => O, | ||
| untransform: (v: O, a: A, superJson: SuperJSON) => I | ||
| untransform: (v: O, a: A, superJson: SuperJSON) => I, | ||
| isDeep: (v: O, superJson: SuperJSON) => boolean | ||
| ) { | ||
| return { | ||
| isApplicable, | ||
| annotation, | ||
| transform, | ||
| untransform, | ||
| isDeep, | ||
| }; | ||
| } |
There was a problem hiding this comment.
isDeep type parameter is O (output) but receives I (input) at the call site
The compositeTransformation helper declares isDeep as:
isDeep: (v: O, superJson: SuperJSON) => booleanwhere O is the transformed/output type. However, in transformValue the function is called with the original value (type I, before transformation):
isDeep: applicableCompositeRule.isDeep(value, superJson), // value is I, not OAll current implementations happen to work correctly despite this mismatch:
classRule,symbolRule,typedArrayRuleignoreventirely (() => true/false)customRulecallsfindApplicable(value)which usesisApplicable— a check on the original typeI— so it still finds the right transformer
But the declared type misleads any future contributor who adds a new composite rule and assumes v is the post-transformation value. The parameter should be typed (v: I, superJson: SuperJSON) => boolean to accurately describe what is passed at runtime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/transformer.ts
Line: 185-199
Comment:
**`isDeep` type parameter is `O` (output) but receives `I` (input) at the call site**
The `compositeTransformation` helper declares `isDeep` as:
```ts
isDeep: (v: O, superJson: SuperJSON) => boolean
```
where `O` is the **transformed/output** type. However, in `transformValue` the function is called with the **original** value (type `I`, before transformation):
```ts
isDeep: applicableCompositeRule.isDeep(value, superJson), // value is I, not O
```
All current implementations happen to work correctly despite this mismatch:
- `classRule`, `symbolRule`, `typedArrayRule` ignore `v` entirely (`() => true/false`)
- `customRule` calls `findApplicable(value)` which uses `isApplicable` — a check on the original type `I` — so it still finds the right transformer
But the declared type misleads any future contributor who adds a new composite rule and assumes `v` is the post-transformation value. The parameter should be typed `(v: I, superJson: SuperJSON) => boolean` to accurately describe what is passed at runtime.
How can I resolve this? If you propose a fix, please make it concise.
|
I removed the earlier There is one known limitation: if a custom transformer returns an object that is also referenced elsewhere in the original structure, both paths overlap. Correctly preserving referential equality in this case would require additional path tracking and ordering during deserialization, which adds significant complexity. For now, I introduced an I’m not sure if this feature fits the scope of the main library. If not, would it be okay for me to maintain and use this as a fork in a public project where this recursive behavior is needed? |
|
Also this is small function that can make path in /**
* Handle inner annotations to make it more compact while remain human readable
* It takes `innerAnnotations` var from outside to mutate by adding recursive annotations to it
*
* Behavior:
* - If new annotation is `undefined` just return with no modification
* - If new annotation is Array (just a node) append it to `innerAnnotations` under passed `key`
* - If new annotation is Object:
* - If only one key is present append it to `innerAnnotations` under passed `key.annotationKey` (Make it compact)
* - If multiple keys are present appent it to `innerAnnotations` under passed `key`
*/
function handleInnerAnnotations(
innerAnnotations: Record<string, MinimisedTree<TypeAnnotation>>,
index: string,
annotation: MinimisedTree<TypeAnnotation>
) {
if (!annotation) return;
const key = escapeKey(index);
if (isArray(annotation)) {
innerAnnotations[key] = annotation;
return;
}
const innerKeys = Object.keys(annotation);
if (innerKeys.length === 1) {
const innerKey = innerKeys[0];
innerAnnotations[key + '.' + innerKey] = annotation[innerKey];
} else {
innerAnnotations[key] = annotation;
}
}it's meant to replace: if (isArray(recursiveResult.annotations)) {
innerAnnotations[escapeKey(index)] = recursiveResult.annotations;
} else if (isPlainObject(recursiveResult.annotations)) {
forEach(recursiveResult.annotations, (tree, key) => {
innerAnnotations[escapeKey(index) + '.' + key] = tree;
});
}However it requires this type change: type Tree<T> = InnerNode<T> | Leaf<T>;
type Leaf<T> = [T];
type InnerNode<T> = [T, MinimisedTree<T>];
export type MinimisedTree<T> =
| Tree<T>
| { [k: string]: MinimisedTree<T> }
| undefined;and this update in const [nodeValue, children] = tree;
if (children) {
if (isArray(children)) {
traverse(children, walker, version, origin);
} else {
forEach(children, (child, key) => {
traverse(child, walker, version, [
...origin,
...parsePath(key, legacyPaths),
]);
});
}
}when used {
"values": {
"user.profile.createdAt": ["Date"],
"user.profile.updatedAt": ["Date"],
"user.settings.theme": [["class", "Theme"]],
"user.settings.language": [["class", "Lan"]],
"user.settings.notifications.email": [["custom", "Email"]],
"user.settings.notifications.location": [["custom", "Location"]]
}
}to: {
"values": {
"user": {
"profile": {
"createdAt": ["Date"],
"updatedAt": ["Date"]
},
"settings": {
"theme": [["class", "Theme"]],
"language": [["class", "Lan"]],
"notifications": {
"email": [["custom", "Email"]],
"location": [["custom", "Location"]]
}
}
}
}
} |
|
|
||
| const equalityGroup = equalityGroups.get( | ||
| stringifyPathWithDepth([childPath, 0]) | ||
| ); | ||
|
|
||
| if (equalityGroup) { | ||
| if (equalityGroup.resolved) return; | ||
| equalityGroup.resolved = true; | ||
| } |
There was a problem hiding this comment.
When a path key in the values tree has multiple segments (e.g. "a.b.date"), the intermediate loop checks whether any intermediate path segment ("a", "a.b") belongs to an equality group. If it does and resolved is false, the group is marked resolved = true and traversal continues. This is correct.
However, the return inside the for loop:
if (equalityGroup.resolved) return; // exits only the forEach callbackexits only the current forEach callback, not the outer function. This means that when a second key like "c.d.date" is processed — where c.d is the duplicate of a.b — the callback returns early, which is the intended skip. But there is a subtle ordering hazard:
If two different keys in the tree share an intermediate segment that belongs to an equality group, the first processed key marks the group resolved, and the second key skips its entire subtree. Any child annotations under the second key are never applied.
Example: imagine keys "shared.x" and "shared.y" where "shared" is in an equality group. After processing "shared.x", the group is marked resolved. When "shared.y" is iterated in forEach, the intermediate check for "shared" finds resolved = true and returns early — the ["shared", "y"] annotation is never applied. This edge case should be covered by a targeted test before merging.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plainer.ts
Line: 64-72
Comment:
**Intermediate-path equality resolution silently drops child annotations**
When a path key in the values tree has multiple segments (e.g. `"a.b.date"`), the intermediate loop checks whether any intermediate path segment (`"a"`, `"a.b"`) belongs to an equality group. If it does and `resolved` is `false`, the group is marked `resolved = true` and traversal continues. This is correct.
However, the `return` inside the `for` loop:
```ts
if (equalityGroup.resolved) return; // exits only the forEach callback
```
exits only the **current `forEach` callback**, not the outer function. This means that when a second key like `"c.d.date"` is processed — where `c.d` is the duplicate of `a.b` — the callback returns early, which is the intended skip. But there is a subtle ordering hazard:
If two **different** keys in the tree share an intermediate segment that belongs to an equality group, the **first** processed key marks the group resolved, and the **second** key skips its entire subtree. Any child annotations under the second key are never applied.
Example: imagine keys `"shared.x"` and `"shared.y"` where `"shared"` is in an equality group. After processing `"shared.x"`, the group is marked resolved. When `"shared.y"` is iterated in `forEach`, the intermediate check for `"shared"` finds `resolved = true` and returns early — the `["shared", "y"]` annotation is never applied. This edge case should be covered by a targeted test before merging.
How can I resolve this? If you propose a fix, please make it concise.
src/plainer.ts
Outdated
| // Apply other referential equality | ||
| for (const path of representatives) { | ||
| const equalityGroup = equalityGroups.get(path)!; | ||
| setReferentialEqualityFn( | ||
| equalityGroup.representative, | ||
| equalityGroup.duplicates | ||
| ); | ||
| } |
There was a problem hiding this comment.
Step 1 of applyMeta eagerly copies representative values to all their duplicates before any type-annotation is applied. For a representative path that has no entry in the values tree, step 2's setValueAnnotationsFn never fires for that path, so the duplicates are permanently set to the raw serialized form rather than the true deserialized value.
Consider adding a test that verifies a dedupe: true scenario where two paths share the same referential-equality group but only one of them carries a values annotation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plainer.ts
Line: 248-255
Comment:
**Pre-application of referential equalities uses stale (pre-transform) values**
Step 1 of `applyMeta` eagerly copies representative values to all their duplicates before any type-annotation is applied. For a representative path that has no entry in the `values` tree, step 2's `setValueAnnotationsFn` never fires for that path, so the duplicates are permanently set to the raw serialized form rather than the true deserialized value.
Consider adding a test that verifies a `dedupe: true` scenario where two paths share the same referential-equality group but only one of them carries a values annotation.
How can I resolve this? If you propose a fix, please make it concise.1 participant
issue #350 code chages
Greptile Summary
This PR adds recursive custom transformations to SuperJSON: a
recursive: trueflag on custom transformers tells the walker to pass the serialized output back through the full SuperJSON pipeline, enabling nested non-JSON values (e.g. aDateinside a custom class) to be annotated and round-tripped without manually callingserializerecursively. It also fixes referential-equality handling for objects that share identity across transformation boundaries, and bumps the wire-format version to2to support a new.$<depth>path suffix that disambiguates annotation depth from path keys.\n\nKey changes:\n-custom-transformer-registry.ts: New discriminated-union typesNonRecursiveCustomTransfomer/RecursiveCustomTransfomerwith overloadedregisterandregisterCustomsignatures.\n-transformer.ts: All transformation rules now carry anisDeepflag;customRuledelegates totransformer.recursive;transformValuereturnsisDeepalongsidevalueandtype.\n-plainer.ts:walkergains asamePathDepthcounter that increments on each recursive transformation at the same JSON path;applyValueAnnotations/applyReferentialEqualityAnnotationsare replaced by a singleapplyMetathat coordinates type-annotation application and referential-equality propagation in one pass.\n-pathstringifier.ts:escapeKeynow escapes$-prefixed keys (to avoid collision with the.$<depth>suffix);parsePathadds adepthSegmentparameter to strip the suffix during parse.\n-accessDeep.ts: Adds array bounds-checking guards.\n-referentialEquality.test.ts: New comprehensive test suite for referential-equality scenarios including recursive transformations,Map/Set, and circular references.\n\nOne P1 issue found: the\\$escape added toparsePathis only stripped for the last path segment. Intermediate segments with$-prefixed keys are pushed into the result array with a literal backslash, causingsetDeepto silently miss the target key during deserialisation. The existing test only exercises$keys at the last position, so this case goes undetected.Confidence Score: 4/5
Not safe to merge until the intermediate-segment
\$-unescape bug inparsePathis fixed; all other concerns are minor.One definite P1 correctness bug: any object whose annotation path contains a
$-prefixed key at a non-terminal position will fail to deserialise correctly (the transformed value is silently dropped). All remaining findings are P2 style/test-design suggestions that do not block functionality.src/pathstringifier.ts — the
parsePathloop must handle the\$escape mid-string, not only at the final segment.Important Files Changed
$-key escaping inescapeKeyand depth-segment stripping inparsePath; the\$escape is not consumed mid-loop, so intermediate$-prefixed path segments round-trip incorrectly.samePathDepthtracking for recursive transformations, mergesapplyValueAnnotations/applyReferentialEqualityAnnotationsintoapplyMeta, and addsReferentialEqualityGroupto co-ordinate annotation + equality application; logic appears sound.isDeepflag to both simple and composite transformation rules; all existing rules assigned sensible values (Error/Set/Map/classRule = true, others = false), andcustomRuledelegates totransformer.recursive.CustomTransfomerintoNonRecursiveCustomTransfomerandRecursiveCustomTransfomervia discriminated union with an overloadedregister; clean type-safe design.applyMeta, bumps serialization version to 2, and addsregisterCustomoverloads for recursive transformers; straightforward wiring changes.v:1tov:2, adds a recursive-custom-transformer round-trip test, and a$-key test; the$-key test doesn't cover intermediate-path$keys, leaving the introduced bug uncaught.SuperJSONinstances that accumulate registrations across tests.metatype with the newMetaObjectexport fromplainer.ts; purely structural cleanup.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["walker(object)"] --> B{isPrimitive?} B -- yes --> Z[return as-is] B -- no --> C[addIdentity + check seenObjects] C --> D{already seen?} D -- "yes, dedupe" --> E[return null] D -- "yes, no dedupe" --> F[return cached result] D -- no --> G{circular?} G -- yes --> H[return null] G -- no --> I["transformValue(object)"] I --> J{match found?} J -- "no match" --> K{isPlainObject or Array?} K -- yes --> L["forEach child → walker(path+index, depth=0)"] K -- no --> Z L --> M[collect innerAnnotations] M --> N[return transformedValue + annotations map] J -- "match, isDeep=false" --> O[return transformedValue + annotation leaf] J -- "match, isDeep=true" --> P["walker(transformedValue, same path, depth+1)"] P --> Q{inner annotations?} Q -- yes --> R["annotations = [type, innerAnnotations]"] Q -- no --> S["annotations = [type]"] R --> T[cache in seenObjects, return] S --> TComments Outside Diff (7)
src/index.test.ts, line 260-268 (link)defaultInstanceregistryEvery test in this file that calls
SuperJSON.registerSerializableClass(User)mutates the globalSuperJSON.defaultInstance(the shared registry). BecauseDoubleIndexedKV.setunconditionally overwrites thekeyToValuemap entry, each newUserclass registered with the identifier"User"silently replaces the previous one:This means the
isInstanceOfSerializableClasscheck still returnstrueforUserAinstances (sincevalueToKey[UserA]exists), but deserialization will useUserBinstead ofUserA.Currently the tests are resilient because each test that round-trips is self-contained (registers and immediately parse/stringify). But as the test suite grows, this fragility is a maintenance risk.
Consider either:
new SuperJSON()instances inside each test instead of the static methodsclear()call onserializableClassRegistryinafterEachThis issue also appears in tests at lines ~291, ~302, ~320, ~328, and ~343.
Prompt To Fix With AI
src/index.test.ts, line 151-171 (link)The following test names contain spelling errors:
'works for recrusive custom registry'→'works for recursive custom registry''throw if serilization/deserialization method is missing'→'throw if serialization/deserialization method is missing''Handles recrusive primitives and non-serilizable classes'→'Handles recursive primitives and non-serializable classes'These typos appear in the test description strings visible in test reports.
Prompt To Fix With AI
src/index.test.ts, line 220-246 (link)SuperJSON.registerCustom(...)delegates toSuperJSON.defaultInstance. The serialization and deserialization, however, useinstance = new SuperJSON({ dedupe: true }), which has its own freshcustomTransformerRegistry. BecauseBoxis not registered oninstance, the transformer is never applied duringinstance.serialize(input). The test does not actually exercise recursive custom transformers withdedupe: true.Change
SuperJSON.registerCustomtoinstance.registerCustomso the transformer is registered on the same instance used for serialization:Prompt To Fix With AI
src/index.test.ts, line 220-246 (link)SuperJSON.registerCustom(...)is a static method bound toSuperJSON.defaultInstance:It registers the
'box-dedupe'transformer only onSuperJSON.defaultInstance.customTransformerRegistry. However, the serialization is performed on a freshinstance = new SuperJSON({ dedupe: true }), which has its own independentcustomTransformerRegistry(initialized asnew CustomTransformerRegistry()per the class field declaration). The fresh instance has no knowledge of'box-dedupe'.As a result, when
instance.serialize(input)walksnew Box(shared):instance.customTransformerRegistryisPlainObjectOrArray(new Box(...))isfalse(Box prototype ≠Object.prototype)sharedreference inside each Box is never tracked by SuperJSON's dedupe logicThe assertion
expect(result.first.value).toBe(result.second.value)will fail because both values are deserialized as separate plain objects.The fix is to register the transformer on
instancerather than the global default:Prompt To Fix With AI
src/pathstringifier.ts, line 27-43 (link)\$escape not consumed — backslash leaks into parsed segmentescapeKey('$foo')produces the string\$foo(literal backslash + dollar). WhenparsePathlater reads that string, the escape-validation block correctly allows\$without throwing, but it does not consume the backslash the way\.is consumed. TheisEscapedDotbranch below handles\.properly (adds.to segment and skips viai++), but there is no equivalentisEscapedDollarbranch. The\character therefore falls through tosegment += charand ends up in the final segment.Concrete trace for key
$foo:setDeep(json, ['\$foo'], ...)then looks for the property'\\$foo'in the object, which doesn't exist — the annotation is silently skipped and the value is never deserialized. Any object with a key starting with$(e.g. MongoDB-style selectors like$gt,$in, or GraphQL variables like$variables) will produce corrupted output when serialized under v=2.A fix needs to add explicit handling for the
\$escape, similar to how\.is handled:This must be placed before the
isEscapedDotcheck so that\$is consumed before the generic fall-through tosegment += char.Prompt To Fix With AI
src/pathstringifier.ts, line 27-43 (link)\$escape sequence is validated but never decodedescapeKeynow encodes keys starting with$as\$key(e.g.$foo→\$foo), butparsePathnever consumes the\before$. The escape check on line 33 allows the character through without erroring, then the backslash falls intosegment += char(line 52) and the next iteration adds$. The decoded segment becomes\$fooinstead of$foo.Concretely: any object with a key starting with
$will silently fail to round-trip in v2 —setDeep/getDeepwill target key\$fooon the JSON object instead of the real key$foo, meaning the annotation is never applied.The fix is to add an
isEscapedDollarbranch analogous to the existingisEscapedDotbranch:Without this fix, any serialized object whose keys start with
$(e.g. MongoDB projections, GraphQL variables, AngularJS bindings) will silently drop their type annotations or referential-equality links.Prompt To Fix With AI
src/pathstringifier.ts, line 24-61 (link)\$escape only stripped from the last path segmentThe
parsePathloop validates\$as a legal escape sequence (line 33) but never consumes it the way\.is consumed (lines 38–43). As a result, when\$appears in an intermediate segment, the literal backslash is added tosegmentand pushed intoresultas\$keyinstead of$key.The corrective code on lines 55–58 (
segment.slice(1)) only runs for the last segment; intermediate segments are pushed raw on line 47. This means any object whose annotation path has a$-prefixed key at a non-terminal position will deserialize incorrectly.Concrete failure:
Serialisation produces annotation key
a.\$middle.b. WhenparsePath('a.\\$middle.b', false, true)is called during deserialisation,\$middleis an intermediate segment and is pushed as\$middle(with literal backslash) instead of$middle.setDeepthen silently misses the key.The
'Handle Doller at start of string correctly'test does not expose this because its$-prefixed keys only appear at the last position in annotation paths.The fix is to handle
\$the same way\.is handled — consume the escape and add just$to the segment:Then the final last-segment special-case on lines 55–58 can be simplified back to
result.push(segment).Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "fix: handle '$' in parse function" | Re-trigger Greptile