Skip to content

Conversation

@jdolle
Copy link
Collaborator

@jdolle jdolle commented Jul 8, 2025

Description

This change began as one to only the adds fields that are necessary for recreating the patched schema using the input schema and changes array, but it has expanded to also add the patch functionality. I found this was necessary in order to verify that the changes provided sufficient data.

This PR modifies the several change types, and modifies the onAdded handlers to also execute equivalent logic to onMutual. It also adds a new rule that can be added to filter out these additional changes, thus keeping changelogs to a minimum. Paths also needed changed to consistently and uniquely map to the node being changed. Previously, some changes referenced a parent's coordinate. Lastly, directives support was dramatically improved and the directive path was modified to include @ to distinguish it from any other node potentially with the same name.

The expansion of "onAdded" has a few down sides and an important up side.

Cons

  1. Increased noise when listing changes
  2. Increased complexity because it blurs the line between changes and additions. Although arguably some of the changes did this already.
  3. It requires adjusting some of the criticality of these changes when they are created from within the onAdded call. Because e.g. adding an interface to newly create object is completely safe, but adding an interface to an existing object is considered dangerous.

Pro

  1. Each change is represented as an event, so it the actual logic and mapping is simple. It's just the number of possible changes that makes this complex.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

See unit tests.

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@jdolle jdolle requested a review from n1ru4l July 8, 2025 17:41
@jdolle jdolle self-assigned this Jul 8, 2025
meta: {
directiveName: oldDirective.name,
oldDirectiveDescription: oldDirective.description ?? null,
directiveName: newDirective.name,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In these cases, the type is being used to extract the name only. It doesn't matter if we pass the new or old directive, since the name is the same. However, since calling these functions onAdd -- the old type can be null, which is why this was changed to pass the new type (newDirective)


{
const change = findFirstChangeByPath(changes, 'enumA.A');
expect(change.criticality.level).toEqual(CriticalityLevel.NonBreaking);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test shows that enum additions also contain all nested changes within that enum, and that those changes are flagged as non-breaking.

@jdolle jdolle force-pushed the type-added-meta branch from 359eea3 to cf7fcfc Compare July 9, 2025 04:40
parentType: type,
field: newField,
}),
directiveUsageAdded(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this filter out the deprecated directive since that's captured by another change?

@jdolle jdolle requested a review from dotansimha July 9, 2025 04:50
@jdolle jdolle changed the title Enhance TypeAdded meta data Diff returns all nested changes for additions Jul 9, 2025
@jdolle jdolle marked this pull request as ready for review July 9, 2025 21:33
@jdolle jdolle requested a review from ardatan July 17, 2025 04:25
@jdolle jdolle changed the title Diff returns all nested changes for additions Add patch package to apply diff changes to schemas Jul 24, 2025
@jdolle jdolle added the ⚙️ work in progress Someone is working on it label Jul 24, 2025
@jdolle jdolle removed the ⚙️ work in progress Someone is working on it label Jul 26, 2025
@jdolle
Copy link
Collaborator Author

jdolle commented Nov 1, 2025

a lot of if {} else {}, these could be replaced by if { .... return } ..., to have some less indent

Adjusted

Copy link
Member

@dotansimha dotansimha left a comment

Choose a reason for hiding this comment

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

LGTM. I added a few minor comments, mainly around release pipeline.
We can proceed once @n1ru4l confirms it's good to go.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants