Skip to content

Commit f379d69

Browse files
leebyronleebyron
committed
Follow ups to #508
Remove usage of Set where simple Object would do. Simplifies type kind description. Sentencify Renames files
1 parent 07ac11a commit f379d69

File tree

4 files changed

+95
-99
lines changed

4 files changed

+95
-99
lines changed

‎src/type/definition.js‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,6 @@ export class GraphQLScalarType {
223223

224224
_scalarConfig: GraphQLScalarTypeConfig<*, *>;
225225

226-
static prettyName: string = 'Scalar';
227-
228226
constructor(config: GraphQLScalarTypeConfig<*, *>) {
229227
invariant(config.name, 'Type must be named.');
230228
assertValidName(config.name);
@@ -326,8 +324,6 @@ export class GraphQLObjectType {
326324
_fields: GraphQLFieldDefinitionMap;
327325
_interfaces: Array<GraphQLInterfaceType>;
328326

329-
static prettyName: string = 'Object';
330-
331327
constructor(config: GraphQLObjectTypeConfig<*>) {
332328
invariant(config.name, 'Type must be named.');
333329
assertValidName(config.name);
@@ -574,8 +570,6 @@ export class GraphQLInterfaceType {
574570
_typeConfig: GraphQLInterfaceTypeConfig;
575571
_fields: GraphQLFieldDefinitionMap;
576572

577-
static prettyName: string = 'Interface';
578-
579573
constructor(config: GraphQLInterfaceTypeConfig) {
580574
invariant(config.name, 'Type must be named.');
581575
assertValidName(config.name);
@@ -647,8 +641,6 @@ export class GraphQLUnionType {
647641
_types: Array<GraphQLObjectType>;
648642
_possibleTypeNames: {[typeName: string]: boolean};
649643

650-
static prettyName: string = 'Union';
651-
652644
constructor(config: GraphQLUnionTypeConfig) {
653645
invariant(config.name, 'Type must be named.');
654646
assertValidName(config.name);
@@ -750,8 +742,6 @@ export class GraphQLEnumType/* <T> */ {
750742
_valueLookup: Map<any/* T */, GraphQLEnumValueDefinition>;
751743
_nameLookup: { [valueName: string]: GraphQLEnumValueDefinition };
752744

753-
static prettyName: string = 'Enum';
754-
755745
constructor(config: GraphQLEnumTypeConfig/* <T> */) {
756746
this.name = config.name;
757747
assertValidName(config.name);
@@ -903,8 +893,6 @@ export class GraphQLInputObjectType {
903893
_typeConfig: InputObjectConfig;
904894
_fields: InputObjectFieldMap;
905895

906-
static prettyName: string = 'InputObject';
907-
908896
constructor(config: InputObjectConfig) {
909897
invariant(config.name, 'Type must be named.');
910898
assertValidName(config.name);

‎src/utilities/__tests__/schemaComparisons-test.js‎ renamed to ‎src/utilities/__tests__/findBreakingChanges-test.js‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
findTypesRemovedFromUnions,
2727
findTypesThatChangedKind,
2828
findValuesRemovedFromEnums,
29-
} from '../schemaComparisons';
29+
} from '../findBreakingChanges';
3030

3131
describe('CheckSchemaBackwardsCompatibility', () => {
3232
const queryType = new GraphQLObjectType({
@@ -66,7 +66,7 @@ describe('CheckSchemaBackwardsCompatibility', () => {
6666
expect(findRemovedTypes(oldSchema, newSchema)).to.eql([
6767
{
6868
type: BreakingChangeType.TYPE_REMOVED,
69-
description: 'Type1 was removed',
69+
description: 'Type1 was removed.',
7070
}
7171
]);
7272
expect(findRemovedTypes(oldSchema, oldSchema)).to.eql([]);
@@ -107,7 +107,7 @@ describe('CheckSchemaBackwardsCompatibility', () => {
107107
expect(findTypesThatChangedKind(oldSchema, newSchema)).to.eql([
108108
{
109109
type: BreakingChangeType.TYPE_CHANGED_KIND,
110-
description: 'Type1 changed from an Interface type to a Union type',
110+
description: 'Type1 changed from an Interface type to a Union type.',
111111
}
112112
]);
113113
});
@@ -169,15 +169,15 @@ describe('CheckSchemaBackwardsCompatibility', () => {
169169
const expectedFieldChanges = [
170170
{
171171
type: BreakingChangeType.FIELD_REMOVED,
172-
description: 'Type1.field2 was removed',
172+
description: 'Type1.field2 was removed.',
173173
},
174174
{
175175
type: BreakingChangeType.FIELD_CHANGED_KIND,
176-
description: 'Type1.field3 changed type from String to Boolean',
176+
description: 'Type1.field3 changed type from String to Boolean.',
177177
},
178178
{
179179
type: BreakingChangeType.FIELD_CHANGED_KIND,
180-
description: 'Type1.field4 changed type from TypeA to TypeB',
180+
description: 'Type1.field4 changed type from TypeA to TypeB.',
181181
},
182182
];
183183
expect(findFieldsThatChangedType(oldSchema, newSchema)).to.eql(
@@ -240,7 +240,7 @@ describe('CheckSchemaBackwardsCompatibility', () => {
240240
expect(findTypesRemovedFromUnions(oldSchema, newSchema)).to.eql([
241241
{
242242
type: BreakingChangeType.TYPE_REMOVED_FROM_UNION,
243-
description: 'Type2 was removed from union type UnionType1',
243+
description: 'Type2 was removed from union type UnionType1.',
244244
},
245245
]);
246246
});
@@ -279,7 +279,7 @@ describe('CheckSchemaBackwardsCompatibility', () => {
279279
expect(findValuesRemovedFromEnums(oldSchema, newSchema)).to.eql([
280280
{
281281
type: BreakingChangeType.VALUE_REMOVED_FROM_ENUM,
282-
description: 'VALUE1 was removed from enum type EnumType1',
282+
description: 'VALUE1 was removed from enum type EnumType1.',
283283
},
284284
]);
285285
});
@@ -381,35 +381,35 @@ describe('CheckSchemaBackwardsCompatibility', () => {
381381
const expectedBreakingChanges = [
382382
{
383383
type: BreakingChangeType.TYPE_REMOVED,
384-
description: 'TypeThatGetsRemoved was removed',
384+
description: 'TypeThatGetsRemoved was removed.',
385385
},
386386
{
387387
type: BreakingChangeType.TYPE_REMOVED,
388-
description: 'TypeInUnion2 was removed',
388+
description: 'TypeInUnion2 was removed.',
389389
},
390390
{
391391
type: BreakingChangeType.TYPE_CHANGED_KIND,
392392
description: 'TypeThatChangesType changed from an Object type to an ' +
393-
'Interface type',
393+
'Interface type.',
394394
},
395395
{
396396
type: BreakingChangeType.FIELD_REMOVED,
397-
description: 'TypeThatHasBreakingFieldChanges.field1 was removed',
397+
description: 'TypeThatHasBreakingFieldChanges.field1 was removed.',
398398
},
399399
{
400400
type: BreakingChangeType.FIELD_CHANGED_KIND,
401401
description: 'TypeThatHasBreakingFieldChanges.field2 changed type ' +
402-
'from String to Boolean',
402+
'from String to Boolean.',
403403
},
404404
{
405405
type: BreakingChangeType.TYPE_REMOVED_FROM_UNION,
406406
description: 'TypeInUnion2 was removed from union type ' +
407-
'UnionTypeThatLosesAType',
407+
'UnionTypeThatLosesAType.',
408408
},
409409
{
410410
type: BreakingChangeType.VALUE_REMOVED_FROM_ENUM,
411411
description: 'VALUE0 was removed from enum type ' +
412-
'EnumTypeThatLosesAValue',
412+
'EnumTypeThatLosesAValue.',
413413
},
414414
];
415415
expect(findBreakingChanges(oldSchema, newSchema)).to.eql(

‎src/utilities/schemaComparisons.js‎ renamed to ‎src/utilities/findBreakingChanges.js‎

Lines changed: 79 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,17 @@
1010

1111
import {
1212
getNamedType,
13+
GraphQLScalarType,
1314
GraphQLEnumType,
1415
GraphQLInputObjectType,
1516
GraphQLInterfaceType,
1617
GraphQLObjectType,
1718
GraphQLUnionType,
1819
} from '../type/definition';
19-
import {
20-
GraphQLSchema,
21-
} from '../type/schema';
20+
21+
import type { GraphQLNamedType } from '../type/definition';
22+
23+
import { GraphQLSchema } from '../type/schema';
2224

2325
export const BreakingChangeType = {
2426
FIELD_CHANGED_KIND: 'FIELD_CHANGED_KIND',
@@ -59,14 +61,19 @@ export function findRemovedTypes(
5961
oldSchema: GraphQLSchema,
6062
newSchema: GraphQLSchema
6163
): Array<BreakingChange> {
62-
const oldTypeNames = Object.keys(oldSchema.getTypeMap());
63-
const newTypes = newSchema.getTypeMap();
64-
return oldTypeNames.filter(typeName => !newTypes[typeName]).map(
65-
type => ({
66-
type: BreakingChangeType.TYPE_REMOVED,
67-
description: `${type} was removed`,
68-
})
69-
);
64+
const oldTypeMap = oldSchema.getTypeMap();
65+
const newTypeMap = newSchema.getTypeMap();
66+
67+
const breakingChanges = [];
68+
Object.keys(oldTypeMap).forEach(typeName => {
69+
if (!newTypeMap[typeName]) {
70+
breakingChanges.push({
71+
type: BreakingChangeType.TYPE_REMOVED,
72+
description: `${typeName} was removed.`,
73+
});
74+
}
75+
});
76+
return breakingChanges;
7077
}
7178

7279
/**
@@ -88,22 +95,36 @@ export function findTypesThatChangedKind(
8895
const oldType = oldTypeMap[typeName];
8996
const newType = newTypeMap[typeName];
9097
if (!(oldType instanceof newType.constructor)) {
91-
breakingChanges.push(
92-
{
93-
type: BreakingChangeType.TYPE_CHANGED_KIND,
94-
description: `${typeName} changed from ` +
95-
`${addArticle(oldType.constructor.prettyName)} type to ` +
96-
`${addArticle(newType.constructor.prettyName)} type`,
97-
}
98-
);
98+
breakingChanges.push({
99+
type: BreakingChangeType.TYPE_CHANGED_KIND,
100+
description: `${typeName} changed from ` +
101+
`${typeKindName(oldType)} to ${typeKindName(newType)}.`
102+
});
99103
}
100104
});
101105
return breakingChanges;
102106
}
103107

104-
function addArticle(typeKind: string): string {
105-
const article = [ 'A','E','I','O' ].includes(typeKind.charAt(0)) ? 'an' : 'a';
106-
return article + ' ' + typeKind;
108+
function typeKindName(type: GraphQLNamedType): string {
109+
if (type instanceof GraphQLScalarType) {
110+
return 'a Scalar type';
111+
}
112+
if (type instanceof GraphQLObjectType) {
113+
return 'an Object type';
114+
}
115+
if (type instanceof GraphQLInterfaceType) {
116+
return 'an Interface type';
117+
}
118+
if (type instanceof GraphQLUnionType) {
119+
return 'a Union type';
120+
}
121+
if (type instanceof GraphQLEnumType) {
122+
return 'an Enum type';
123+
}
124+
if (type instanceof GraphQLInputObjectType) {
125+
return 'an Input type';
126+
}
127+
throw new TypeError('Unknown type ' + type.constructor.name);
107128
}
108129

109130
/**
@@ -136,27 +157,22 @@ export function findFieldsThatChangedType(
136157
Object.keys(oldTypeFieldsDef).forEach(fieldName => {
137158
// Check if the field is missing on the type in the new schema.
138159
if (!(fieldName in newTypeFieldsDef)) {
139-
breakingFieldChanges.push(
140-
{
141-
type: BreakingChangeType.FIELD_REMOVED,
142-
description: `${typeName}.${fieldName} was removed`,
143-
}
144-
);
160+
breakingFieldChanges.push({
161+
type: BreakingChangeType.FIELD_REMOVED,
162+
description: `${typeName}.${fieldName} was removed.`,
163+
});
145164
} else {
146165
// Check if the field's type has changed in the new schema.
147166
const oldFieldType = getNamedType(oldTypeFieldsDef[fieldName].type);
148167
const newFieldType = getNamedType(newTypeFieldsDef[fieldName].type);
149-
if (
150-
oldFieldType && newFieldType &&
151-
(oldFieldType.name !== newFieldType.name)
152-
) {
153-
breakingFieldChanges.push(
154-
{
155-
type: BreakingChangeType.FIELD_CHANGED_KIND,
156-
description: `${typeName}.${fieldName} changed type from ` +
157-
`${oldFieldType.name} to ${newFieldType.name}`,
158-
}
159-
);
168+
if (oldFieldType &&
169+
newFieldType &&
170+
oldFieldType.name !== newFieldType.name) {
171+
breakingFieldChanges.push({
172+
type: BreakingChangeType.FIELD_CHANGED_KIND,
173+
description: `${typeName}.${fieldName} changed type from ` +
174+
`${oldFieldType.name} to ${newFieldType.name}.`,
175+
});
160176
}
161177
}
162178
});
@@ -179,24 +195,20 @@ export function findTypesRemovedFromUnions(
179195
Object.keys(oldTypeMap).forEach(typeName => {
180196
const oldType = oldTypeMap[typeName];
181197
const newType = newTypeMap[typeName];
182-
if (
183-
!(oldType instanceof GraphQLUnionType) ||
184-
!(newType instanceof GraphQLUnionType)
185-
) {
198+
if (!(oldType instanceof GraphQLUnionType) ||
199+
!(newType instanceof GraphQLUnionType)) {
186200
return;
187201
}
188-
const typeNamesInNewUnion = new Set(
189-
newType.getTypes().map(type => type.name)
190-
);
191-
oldType.getTypes().forEach(typeInOldUnion => {
192-
if (!typeNamesInNewUnion.has(typeInOldUnion.name)) {
193-
typesRemovedFromUnion.push(
194-
{
195-
type: BreakingChangeType.TYPE_REMOVED_FROM_UNION,
196-
description: `${typeInOldUnion.name} was removed from union ` +
197-
`type ${typeName}`,
198-
}
199-
);
202+
const typeNamesInNewUnion = Object.create(null);
203+
newType.getTypes().forEach(type => {
204+
typeNamesInNewUnion[type.name] = true;
205+
});
206+
oldType.getTypes().forEach(type => {
207+
if (!typeNamesInNewUnion[type.name]) {
208+
typesRemovedFromUnion.push({
209+
type: BreakingChangeType.TYPE_REMOVED_FROM_UNION,
210+
description: `${type.name} was removed from union type ${typeName}.`
211+
});
200212
}
201213
});
202214
});
@@ -218,24 +230,20 @@ export function findValuesRemovedFromEnums(
218230
Object.keys(oldTypeMap).forEach(typeName => {
219231
const oldType = oldTypeMap[typeName];
220232
const newType = newTypeMap[typeName];
221-
if (
222-
!(oldType instanceof GraphQLEnumType) ||
223-
!(newType instanceof GraphQLEnumType)
224-
) {
233+
if (!(oldType instanceof GraphQLEnumType) ||
234+
!(newType instanceof GraphQLEnumType)) {
225235
return;
226236
}
227-
const valuesInNewEnum = new Set(
228-
newType.getValues().map(value => value.name)
229-
);
230-
oldType.getValues().forEach(valueInOldEnum => {
231-
if (!valuesInNewEnum.has(valueInOldEnum.name)) {
232-
valuesRemovedFromEnums.push(
233-
{
234-
type: BreakingChangeType.VALUE_REMOVED_FROM_ENUM,
235-
description: `${valueInOldEnum.name} was removed from enum ` +
236-
`type ${typeName}`,
237-
}
238-
);
237+
const valuesInNewEnum = Object.create(null);
238+
newType.getValues().forEach(value => {
239+
valuesInNewEnum[value.name] = true;
240+
});
241+
oldType.getValues().forEach(value => {
242+
if (!valuesInNewEnum[value.name]) {
243+
valuesRemovedFromEnums.push({
244+
type: BreakingChangeType.VALUE_REMOVED_FROM_ENUM,
245+
description: `${value.name} was removed from enum type ${typeName}.`
246+
});
239247
}
240248
});
241249
});

‎src/utilities/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,4 +62,4 @@ export {
6262
export { assertValidName } from './assertValidName';
6363

6464
// Compares two GraphQLSchemas and detects breaking changes.
65-
export { findBreakingChanges } from './schemaComparisons';
65+
export { findBreakingChanges } from './findBreakingChanges';

0 commit comments

Comments
 (0)