Skip to content

Commit 69ccbc3

Browse files
elicwhitefacebook-github-bot
authored andcommitted
Allow union changes when the new element is in the middle of the union (facebook#50117)
Summary: Pull Request resolved: facebook#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
1 parent 2facea6 commit 69ccbc3

File tree

4 files changed

+41
-39
lines changed

4 files changed

+41
-39
lines changed

packages/react-native-compatibility-check/src/TypeDiffing.js

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import type {
1212
ComparisonResult,
1313
FunctionComparisonResult,
1414
MembersComparisonResult,
15+
PositionalComparisonResult,
1516
PropertiesComparisonResult,
1617
TypeComparisonError,
1718
} from './ComparisonResult';
@@ -905,7 +906,7 @@ export function compareStringLiteralUnionTypes(
905906
olderType: StringLiteralUnionTypeAnnotation,
906907
): ComparisonResult {
907908
const results = compareArrayOfTypes(
908-
true, // Fixed order
909+
false, // Fixed order
909910
false, // Can grow/shrink at the end
910911
newerType.types,
911912
olderType.types,
@@ -928,28 +929,30 @@ export function compareStringLiteralUnionTypes(
928929
'Unexpected inline objects/functions in string literal union',
929930
);
930931
}
932+
if (
933+
results.addedElements.length <= 0 &&
934+
results.removedElements.length <= 0
935+
) {
936+
throw new Error('string union returned unexpected set of changes');
937+
}
938+
939+
const changeLog: PositionalComparisonResult = {
940+
typeKind: 'stringUnion',
941+
nestedChanges: [],
942+
};
943+
931944
if (results.addedElements.length > 0) {
932-
return {
933-
status: 'positionalTypeChange',
934-
changeLog: {
935-
typeKind: 'stringUnion',
936-
nestedChanges: [],
937-
addedElements: results.addedElements,
938-
},
939-
};
945+
changeLog.addedElements = results.addedElements;
940946
}
947+
941948
if (results.removedElements.length > 0) {
942-
return {
943-
status: 'positionalTypeChange',
944-
changeLog: {
945-
typeKind: 'stringUnion',
946-
nestedChanges: [],
947-
removedElements: results.removedElements,
948-
},
949-
};
949+
changeLog.removedElements = results.removedElements;
950950
}
951-
console.log(JSON.stringify(results));
952-
throw new Error('string union returned unexpected set of changes');
951+
952+
return {
953+
status: 'positionalTypeChange',
954+
changeLog,
955+
};
953956
case 'matching':
954957
return {status: 'matching'};
955958
default:

packages/react-native-compatibility-check/src/__tests__/TypeDiffing-test.js

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,16 @@ describe('compareTypes on string literal unions', () => {
10101010
nativeTypeDiffingTypesAliases,
10111011
nativeTypeDiffingTypesAliases,
10121012
),
1013-
).toHaveErrorWithMessage('Subtype of union at position 1 did not match');
1013+
).toEqual(
1014+
expect.objectContaining({
1015+
status: 'positionalTypeChange',
1016+
changeLog: expect.objectContaining({
1017+
typeKind: 'stringUnion',
1018+
addedElements: expect.arrayContaining([expect.any(Array)]),
1019+
removedElements: expect.arrayContaining([expect.any(Array)]),
1020+
}),
1021+
}),
1022+
);
10141023
});
10151024
});
10161025

packages/react-native-compatibility-check/src/__tests__/__fixtures__/native-component-with-props-union-added/NativeComponent.js.flow

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import codegenNativeComponent from 'react-native/Libraries/Utilities/codegenNati
1616

1717
export type Props = $ReadOnly<{
1818
...ViewProps,
19-
size?: WithDefault<'small' | 'large' | 'huge', 'small'>,
19+
size?: WithDefault<'small' | 'huge' | 'large', 'small'>,
2020
}>;
2121

2222
export default (codegenNativeComponent<Props>(

packages/react-native-compatibility-check/src/__tests__/__snapshots__/ErrorFormatting-test.js.snap

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ Object {
196196
Object {
197197
"errorCode": "addedUnionCases",
198198
"message": "NativeComponent.sizes: Union added items, but native will not expect/support them
199-
-- position 3 huge",
199+
-- position 2 huge",
200200
},
201201
],
202202
},
@@ -237,7 +237,7 @@ Object {
237237
Object {
238238
"errorCode": "addedUnionCases",
239239
"message": "NativeComponent.size: Union added items, but native will not expect/support them
240-
-- position 3 huge",
240+
-- position 1 huge",
241241
},
242242
],
243243
},
@@ -709,7 +709,7 @@ Object {
709709
Object {
710710
"errorCode": "addedUnionCases",
711711
"message": "NativeModuleTest.exampleFunction parameter 0: Union added items, but native will not expect/support them
712-
-- position 4 d",
712+
-- position 3 d",
713713
},
714714
],
715715
},
@@ -725,20 +725,10 @@ Object {
725725
"framework": "ReactNative",
726726
"incompatibleSpecs": Array [
727727
Object {
728-
"errorCode": "incompatibleTypes",
729-
"message": "NativeModuleTest: Object contained a property with a type mismatch
730-
-- exampleFunction: has conflicting type changes
731-
--new: (a: (a | b | c), b: number)=>void
732-
--old: (a: (a | '0' | '1' | 'a long string'), b: number)=>void
733-
Parameter at index 0 did not match
734-
--new: (a: (a | b | c), b: number)=>void
735-
--old: (a: (a | '0' | '1' | 'a long string'), b: number)=>void
736-
Subtype of union at position 1 did not match
737-
--new: (a | b | c)
738-
--old: (a | '0' | '1' | 'a long string')
739-
String literals are not equal
740-
--new: b
741-
--old: '0'",
728+
"errorCode": "addedUnionCases",
729+
"message": "NativeModuleTest.exampleFunction parameter 0: Union added items, but native will not expect/support them
730+
-- position 1 b
731+
-- position 2 c",
742732
},
743733
],
744734
},
@@ -756,7 +746,7 @@ Object {
756746
Object {
757747
"errorCode": "removedUnionCases",
758748
"message": "NativeModuleTest.getConstants.exampleConstant: Union removed items, but native may still provide them
759-
-- position 4 d",
749+
-- position 3 d",
760750
},
761751
],
762752
},

0 commit comments

Comments
 (0)