Skip to content

Commit 3948be9

Browse files
eapacheeapache
committed
Refactor for better typing
Strongly type the return objects, use invariants in a way to satisfy flow, and DRY up one common function.
1 parent eb15335 commit 3948be9

File tree

2 files changed

+102
-81
lines changed

2 files changed

+102
-81
lines changed

‎src/utilities/__tests__/findDescriptionChanges-test.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ describe('findDescriptionChanges', () => {
4343
query: queryType,
4444
types: [typeNew],
4545
});
46-
expect(findDescriptionChanges(oldSchema, newSchema)).to.eql([
47-
'Description added on type Type.',
48-
]);
46+
expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql(
47+
'Description added on TYPE Type.',
48+
);
4949
expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]);
5050
expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]);
5151
});
@@ -67,9 +67,9 @@ describe('findDescriptionChanges', () => {
6767
query: queryType,
6868
types: [type],
6969
});
70-
expect(findDescriptionChanges(oldSchema, newSchema)).to.eql([
71-
'Description added on new type Type.',
72-
]);
70+
expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql(
71+
'New TYPE Type added with description.',
72+
);
7373
expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]);
7474
expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]);
7575
});

‎src/utilities/findDescriptionChanges.js‎

Lines changed: 96 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,28 @@ import {
99
} from '../type/definition';
1010
import type { GraphQLFieldMap } from '../type/definition';
1111
import { GraphQLSchema } from '../type/schema';
12+
import invariant from '../jsutils/invariant';
13+
14+
export const DescribedObjectType = {
15+
FIELD: 'FIELD',
16+
TYPE: 'TYPE',
17+
ARGUMENT: 'ARGUMENT',
18+
ENUM_VALUE: 'ENUM_VALUE',
19+
};
20+
21+
export const DescriptionChangeType = {
22+
OBJECT_ADDED: 'OBJECT_ADDED',
23+
DESCRIPTION_ADDED: 'DESCRIPTION_ADDED',
24+
DESCRIPTION_CHANGED: 'DESCRIPTION_CHANGED',
25+
};
26+
27+
export type DescriptionChange = {
28+
object: $Keys<typeof DescribedObjectType>,
29+
change: $Keys<typeof DescriptionChangeType>,
30+
description: string,
31+
oldThing: any,
32+
newThing: any,
33+
};
1234

1335
/**
1436
* Given two schemas, returns an Array containing descriptions of any
@@ -17,37 +39,32 @@ import { GraphQLSchema } from '../type/schema';
1739
export function findDescriptionChanges(
1840
oldSchema: GraphQLSchema,
1941
newSchema: GraphQLSchema,
20-
): Array<string> {
42+
): Array<DescriptionChange> {
2143
const oldTypeMap = oldSchema.getTypeMap();
2244
const newTypeMap = newSchema.getTypeMap();
2345

24-
const descriptionChanges: Array<string> = [];
46+
const descriptionChanges: Array<?DescriptionChange> = [];
2547

2648
Object.keys(newTypeMap).forEach(typeName => {
2749
const oldType = oldTypeMap[typeName];
2850
const newType = newTypeMap[typeName];
2951

30-
if (newType.description) {
31-
if (!oldType) {
32-
descriptionChanges.push(
33-
`Description added on new type ${newType.name}.`,
34-
);
35-
} else if (!oldType.description) {
36-
descriptionChanges.push(`Description added on type ${newType.name}.`);
37-
} else if (oldType.description !== newType.description) {
38-
descriptionChanges.push(`Description changed on type ${newType.name}.`);
39-
}
40-
}
41-
42-
if (oldType && !(newType instanceof oldType.constructor)) {
43-
return;
44-
}
52+
descriptionChanges.push(
53+
generateDescriptionChange(newType, oldType, DescribedObjectType.TYPE),
54+
);
4555

4656
if (
4757
newType instanceof GraphQLObjectType ||
4858
newType instanceof GraphQLInterfaceType ||
4959
newType instanceof GraphQLInputObjectType
5060
) {
61+
invariant(
62+
!oldType ||
63+
oldType instanceof GraphQLObjectType ||
64+
oldType instanceof GraphQLInterfaceType ||
65+
oldType instanceof GraphQLInputObjectType,
66+
'Expected oldType to also have fields',
67+
);
5168
const oldTypeFields: ?GraphQLFieldMap<*, *> = oldType
5269
? oldType.getFields()
5370
: null;
@@ -57,21 +74,13 @@ export function findDescriptionChanges(
5774
const oldField = oldTypeFields ? oldTypeFields[fieldName] : null;
5875
const newField = newTypeFields[fieldName];
5976

60-
if (newField.description) {
61-
if (!oldField) {
62-
descriptionChanges.push(
63-
`Description added on new field ${newType.name}.${newField.name}`,
64-
);
65-
} else if (!oldField.description) {
66-
descriptionChanges.push(
67-
`Description added on field ${newType.name}.${newField.name}.`,
68-
);
69-
} else if (oldField.description !== newField.description) {
70-
descriptionChanges.push(
71-
`Description changed on field ${newType.name}.${newField.name}.`,
72-
);
73-
}
74-
}
77+
descriptionChanges.push(
78+
generateDescriptionChange(
79+
newField,
80+
oldField,
81+
DescribedObjectType.FIELD,
82+
),
83+
);
7584

7685
if (!newField.args) {
7786
return;
@@ -82,61 +91,73 @@ export function findDescriptionChanges(
8291
? oldField.args.find(arg => arg.name === newArg.name)
8392
: null;
8493

85-
if (newArg.description) {
86-
if (!oldArg) {
87-
descriptionChanges.push(
88-
`Description added on new arg ${newType.name}.${
89-
newField.name
90-
}.${newArg.name}.`,
91-
);
92-
} else if (!oldArg.description) {
93-
descriptionChanges.push(
94-
`Description added on arg ${newType.name}.${newField.name}.${
95-
newArg.name
96-
}.`,
97-
);
98-
} else if (oldArg.description !== newArg.description) {
99-
descriptionChanges.push(
100-
`Description changed on arg ${newType.name}.${newField.name}.${
101-
newArg.name
102-
}.`,
103-
);
104-
}
105-
}
94+
descriptionChanges.push(
95+
generateDescriptionChange(
96+
newArg,
97+
oldArg,
98+
DescribedObjectType.ARGUMENT,
99+
),
100+
);
106101
});
107102
});
108103
} else if (newType instanceof GraphQLEnumType) {
104+
invariant(
105+
!oldType || oldType instanceof GraphQLEnumType,
106+
'Expected oldType to also have values',
107+
);
109108
const oldValues = oldType ? oldType.getValues() : null;
110109
const newValues = newType.getValues();
111110
newValues.forEach(newValue => {
112111
const oldValue = oldValues
113112
? oldValues.find(value => value.name === newValue.name)
114113
: null;
115114

116-
if (newValue.description) {
117-
if (!oldValue) {
118-
descriptionChanges.push(
119-
`Description added on enum value ${newType.name}.${
120-
newValue.name
121-
}.`,
122-
);
123-
} else if (!oldValue.description) {
124-
descriptionChanges.push(
125-
`Description added on enum value ${newType.name}.${
126-
newValue.name
127-
}.`,
128-
);
129-
} else if (oldValue.description !== newValue.description) {
130-
descriptionChanges.push(
131-
`Description changed on enum value ${newType.name}.${
132-
newValue.name
133-
}.`,
134-
);
135-
}
136-
}
115+
descriptionChanges.push(
116+
generateDescriptionChange(
117+
newValue,
118+
oldValue,
119+
DescribedObjectType.ENUM_VALUE,
120+
),
121+
);
137122
});
138123
}
139124
});
140125

141-
return descriptionChanges;
126+
return descriptionChanges.filter(Boolean);
127+
}
128+
129+
function generateDescriptionChange(
130+
newThing,
131+
oldThing,
132+
objectType: $Keys<typeof DescribedObjectType>,
133+
): ?DescriptionChange {
134+
if (!newThing.description) {
135+
return;
136+
}
137+
138+
if (!oldThing) {
139+
return {
140+
object: objectType,
141+
change: DescriptionChangeType.OBJECT_ADDED,
142+
oldThing,
143+
newThing,
144+
description: `New ${objectType} ${newThing.name} added with description.`,
145+
};
146+
} else if (!oldThing.description) {
147+
return {
148+
object: objectType,
149+
change: DescriptionChangeType.DESCRIPTION_ADDED,
150+
oldThing,
151+
newThing,
152+
description: `Description added on ${objectType} ${newThing.name}.`,
153+
};
154+
} else if (oldThing.description !== newThing.description) {
155+
return {
156+
object: objectType,
157+
change: DescriptionChangeType.DESCRIPTION_CHANGED,
158+
oldThing,
159+
newThing,
160+
description: `Description changed on ${objectType} ${newThing.name}.`,
161+
};
162+
}
142163
}

0 commit comments

Comments
 (0)