From 69ccbc39438d599308b8d98c0dcf72d3bae1bf41 Mon Sep 17 00:00:00 2001 From: Eli White Date: Wed, 19 Mar 2025 11:33:08 -0700 Subject: [PATCH] Allow union changes when the new element is in the middle of the union (#50117) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/50117 D70870978 failed the compat check because it modified a union by removing an element in the middle: ``` 'global' | 'self' ``` from ``` 'global' | 'application' | 'self' ``` This caused the compat check to complain that index 1 in both unions: `self` didn't match `application` and thus it was a type incompatibility. We should have been comparing these as an unsorted array of options, which first sorts, then treats differences as added/removed elements instead of incompatbile elements. If in the example above the removed element was the last one from the union, it would have been fine. Once these are classified as added/removed, the VersionDiffer is able to check whether that change is allowed in fromNative or toNative. Changelog: [General][Fixed] Compatibility Check: Allow union changes when the new element is in the middle of the union Reviewed By: makovkastar Differential Revision: D71433054 fbshipit-source-id: 20a73f0ba0576daf30cec97bae969b31baf7f468 --- .../src/TypeDiffing.js | 41 ++++++++++--------- .../src/__tests__/TypeDiffing-test.js | 11 ++++- .../NativeComponent.js.flow | 2 +- .../ErrorFormatting-test.js.snap | 26 ++++-------- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/packages/react-native-compatibility-check/src/TypeDiffing.js b/packages/react-native-compatibility-check/src/TypeDiffing.js index abd3310fe1d..405b21e1d80 100644 --- a/packages/react-native-compatibility-check/src/TypeDiffing.js +++ b/packages/react-native-compatibility-check/src/TypeDiffing.js @@ -12,6 +12,7 @@ import type { ComparisonResult, FunctionComparisonResult, MembersComparisonResult, + PositionalComparisonResult, PropertiesComparisonResult, TypeComparisonError, } from './ComparisonResult'; @@ -905,7 +906,7 @@ export function compareStringLiteralUnionTypes( olderType: StringLiteralUnionTypeAnnotation, ): ComparisonResult { const results = compareArrayOfTypes( - true, // Fixed order + false, // Fixed order false, // Can grow/shrink at the end newerType.types, olderType.types, @@ -928,28 +929,30 @@ export function compareStringLiteralUnionTypes( 'Unexpected inline objects/functions in string literal union', ); } + if ( + results.addedElements.length <= 0 && + results.removedElements.length <= 0 + ) { + throw new Error('string union returned unexpected set of changes'); + } + + const changeLog: PositionalComparisonResult = { + typeKind: 'stringUnion', + nestedChanges: [], + }; + if (results.addedElements.length > 0) { - return { - status: 'positionalTypeChange', - changeLog: { - typeKind: 'stringUnion', - nestedChanges: [], - addedElements: results.addedElements, - }, - }; + changeLog.addedElements = results.addedElements; } + if (results.removedElements.length > 0) { - return { - status: 'positionalTypeChange', - changeLog: { - typeKind: 'stringUnion', - nestedChanges: [], - removedElements: results.removedElements, - }, - }; + changeLog.removedElements = results.removedElements; } - console.log(JSON.stringify(results)); - throw new Error('string union returned unexpected set of changes'); + + return { + status: 'positionalTypeChange', + changeLog, + }; case 'matching': return {status: 'matching'}; default: diff --git a/packages/react-native-compatibility-check/src/__tests__/TypeDiffing-test.js b/packages/react-native-compatibility-check/src/__tests__/TypeDiffing-test.js index 55e0a114fcb..e223644aaf9 100644 --- a/packages/react-native-compatibility-check/src/__tests__/TypeDiffing-test.js +++ b/packages/react-native-compatibility-check/src/__tests__/TypeDiffing-test.js @@ -1010,7 +1010,16 @@ describe('compareTypes on string literal unions', () => { nativeTypeDiffingTypesAliases, nativeTypeDiffingTypesAliases, ), - ).toHaveErrorWithMessage('Subtype of union at position 1 did not match'); + ).toEqual( + expect.objectContaining({ + status: 'positionalTypeChange', + changeLog: expect.objectContaining({ + typeKind: 'stringUnion', + addedElements: expect.arrayContaining([expect.any(Array)]), + removedElements: expect.arrayContaining([expect.any(Array)]), + }), + }), + ); }); }); diff --git a/packages/react-native-compatibility-check/src/__tests__/__fixtures__/native-component-with-props-union-added/NativeComponent.js.flow b/packages/react-native-compatibility-check/src/__tests__/__fixtures__/native-component-with-props-union-added/NativeComponent.js.flow index 2a4fcf90d8f..bf132c85480 100644 --- a/packages/react-native-compatibility-check/src/__tests__/__fixtures__/native-component-with-props-union-added/NativeComponent.js.flow +++ b/packages/react-native-compatibility-check/src/__tests__/__fixtures__/native-component-with-props-union-added/NativeComponent.js.flow @@ -16,7 +16,7 @@ import codegenNativeComponent from 'react-native/Libraries/Utilities/codegenNati export type Props = $ReadOnly<{ ...ViewProps, - size?: WithDefault<'small' | 'large' | 'huge', 'small'>, + size?: WithDefault<'small' | 'huge' | 'large', 'small'>, }>; export default (codegenNativeComponent( diff --git a/packages/react-native-compatibility-check/src/__tests__/__snapshots__/ErrorFormatting-test.js.snap b/packages/react-native-compatibility-check/src/__tests__/__snapshots__/ErrorFormatting-test.js.snap index 05f5b3a3bc4..6cddfacb345 100644 --- a/packages/react-native-compatibility-check/src/__tests__/__snapshots__/ErrorFormatting-test.js.snap +++ b/packages/react-native-compatibility-check/src/__tests__/__snapshots__/ErrorFormatting-test.js.snap @@ -196,7 +196,7 @@ Object { Object { "errorCode": "addedUnionCases", "message": "NativeComponent.sizes: Union added items, but native will not expect/support them - -- position 3 huge", + -- position 2 huge", }, ], }, @@ -237,7 +237,7 @@ Object { Object { "errorCode": "addedUnionCases", "message": "NativeComponent.size: Union added items, but native will not expect/support them - -- position 3 huge", + -- position 1 huge", }, ], }, @@ -709,7 +709,7 @@ Object { Object { "errorCode": "addedUnionCases", "message": "NativeModuleTest.exampleFunction parameter 0: Union added items, but native will not expect/support them - -- position 4 d", + -- position 3 d", }, ], }, @@ -725,20 +725,10 @@ Object { "framework": "ReactNative", "incompatibleSpecs": Array [ Object { - "errorCode": "incompatibleTypes", - "message": "NativeModuleTest: Object contained a property with a type mismatch - -- exampleFunction: has conflicting type changes - --new: (a: (a | b | c), b: number)=>void - --old: (a: (a | '0' | '1' | 'a long string'), b: number)=>void - Parameter at index 0 did not match - --new: (a: (a | b | c), b: number)=>void - --old: (a: (a | '0' | '1' | 'a long string'), b: number)=>void - Subtype of union at position 1 did not match - --new: (a | b | c) - --old: (a | '0' | '1' | 'a long string') - String literals are not equal - --new: b - --old: '0'", + "errorCode": "addedUnionCases", + "message": "NativeModuleTest.exampleFunction parameter 0: Union added items, but native will not expect/support them + -- position 1 b + -- position 2 c", }, ], }, @@ -756,7 +746,7 @@ Object { Object { "errorCode": "removedUnionCases", "message": "NativeModuleTest.getConstants.exampleConstant: Union removed items, but native may still provide them - -- position 4 d", + -- position 3 d", }, ], },