Skip to content

Conversation

@ricokahler
Copy link
Contributor

@ricokahler ricokahler commented Jun 4, 2025

This PR refactors the internal serializePatches function to use a recursive approach. The primary goal of this change is to better preserve the original order of operations as they are generated by diffItem when they are grouped into SanityPatchOperations.

Previously, serializePatches would filter all patches by type (set, unset, insert, dmp) and then combine them. This could lead to a reordering of operations if, for example, a set operation was diffed before an unset operation that should logically follow it.

The new recursive approach processes patches one by one:

  • It attempts to merge consecutive patches of the same type (e.g., multiple set operations can be combined into a single set: {...} object).
  • If a patch of a different type is encountered, the current accumulated patch operation object is emitted, and a new one is started for the new type.
  • insert operations always result in a new patch object being emitted, as they are not typically batched with other operation types in the same way.

Key Changes:

  • Recursive serializePatches:
    • The serializePatches function now takes an optional curr?: SanityPatchOperation parameter to carry the currently accumulating patch object.
    • It processes the head of the patches array:
      • For set and diffMatchPatch: If curr is undefined or of a different type, a new curr is started. Otherwise, the operation is merged into the existing curr.
      • For unset: Similar logic to set, but merges into curr.unset[].
      • For insert: If curr exists, it's emitted first, then the insert operation is emitted as a new patch object.
    • This ensures that if the diffing process generates, for example, set, then unset, then set again, these will be serialized into distinct patch operation objects, preserving that logical sequence.
  • Test Snapshot Updates:
    • Snapshots have been updated to reflect the new serialization order. In many cases, this results in more individual patch objects within a SanityPatchMutation[] array where previously operations might have been grouped less granularly or in a different order. For example, a set followed by a diffMatchPatch will now be two separate entries in the SanityPatchMutation[] array if they were generated in that order by diffItem.

Rationale:

  • Order Preservation: Ensures that the sequence of operations (e.g., an unset happening before a set on a related path) as determined by the diffing logic is maintained in the serialized output. This can be important for the logical application of patches, especially in more complex scenarios or when patches might be manipulated or inspected before application.
  • Clearer Grouping: While potentially leading to more patch objects in the array, the grouping is now more directly reflective of the sequence of changes.
  • Foundation for Future Enhancements: This recursive structure might be more adaptable for future features that require more nuanced control over patch grouping or ordering (e.g., reordering patches).

This change primarily affects the internal serialization and the structure of the output array in snapshots, aiming for a more robust and order-preserving serialization.

Copy link
Contributor Author

ricokahler commented Jun 4, 2025

Copy link
Member

@rexxars rexxars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear to me if this actually fixes any current bugs? I understand that it's needed for later changes, but if there are potential bugs in the current implementation, we should ensure this one has a failing test (that gets fixed by this PR)

@ricokahler ricokahler force-pushed the v6_sequential-patch-serialization branch from dda33fd to d4a9699 Compare June 6, 2025 21:57
@ricokahler ricokahler force-pushed the v6_replace-diffitem-with-diffvalue branch from 61a3ffc to 5067260 Compare June 6, 2025 21:57
@ricokahler ricokahler force-pushed the v6_sequential-patch-serialization branch from d4a9699 to 7c922f7 Compare June 6, 2025 22:00
Copy link
Contributor Author

ricokahler commented Jun 13, 2025

Merge activity

  • Jun 13, 7:24 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 13, 7:34 PM UTC: Graphite rebased this pull request as part of a merge.
  • Jun 13, 7:36 PM UTC: @ricokahler merged this pull request with Graphite.

@ricokahler ricokahler changed the base branch from v6_replace-diffitem-with-diffvalue to graphite-base/42 June 13, 2025 19:31
@ricokahler ricokahler changed the base branch from graphite-base/42 to main June 13, 2025 19:33
Change patch serialization from batch processing to sequential processing
to preserve the original order of operations while still optimizing
consecutive compatible patches.

- Process patches one by one instead of grouping by type
- Combine consecutive set/diffMatchPatch and unset operations
- Keep insert operations separate to maintain precision
- Ensure patch application order matches generation order
@ricokahler ricokahler force-pushed the v6_sequential-patch-serialization branch from 7c922f7 to f391c3c Compare June 13, 2025 19:34
@ricokahler ricokahler merged commit 103d640 into main Jun 13, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants