Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a utility to detect description changes #1127

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions src/utilities/__tests__/findDescriptionChanges-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
/**
* Copyright (c) 2016, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/

import { expect } from 'chai';
import { describe, it } from 'mocha';
import { GraphQLObjectType, GraphQLSchema, GraphQLString } from '../../type';
import { findDescriptionChanges } from '../findDescriptionChanges';

describe('findDescriptionChanges', () => {
const queryType = new GraphQLObjectType({
name: 'Query',
fields: {
field1: { type: GraphQLString },
},
});

it('should detect if a description was added to a type', () => {
const typeOld = new GraphQLObjectType({
name: 'Type',
fields: {
field1: { type: GraphQLString },
},
});
const typeNew = new GraphQLObjectType({
name: 'Type',
description: 'Something rather',
fields: {
field1: { type: GraphQLString },
},
});

const oldSchema = new GraphQLSchema({
query: queryType,
types: [typeOld],
});
const newSchema = new GraphQLSchema({
query: queryType,
types: [typeNew],
});
expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql(
'Description added on TYPE Type.',
);
expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]);
expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]);
});

it('should detect if a description was changed on a type', () => {
const typeOld = new GraphQLObjectType({
name: 'Type',
description: 'Something rather',
fields: {
field1: { type: GraphQLString },
},
});
const typeNew = new GraphQLObjectType({
name: 'Type',
description: 'Something else',
fields: {
field1: { type: GraphQLString },
},
});

const oldSchema = new GraphQLSchema({
query: queryType,
types: [typeOld],
});
const newSchema = new GraphQLSchema({
query: queryType,
types: [typeNew],
});
expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql(
'Description changed on TYPE Type.',
);
expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]);
expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]);
});

it('should detect if a type with a description was added', () => {
const type = new GraphQLObjectType({
name: 'Type',
description: 'Something rather',
fields: {
field1: { type: GraphQLString },
},
});

const oldSchema = new GraphQLSchema({
query: queryType,
types: [],
});
const newSchema = new GraphQLSchema({
query: queryType,
types: [type],
});
expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql(
'New TYPE Type added with description.',
);
expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]);
expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]);
});

it('should detect if a field with a description was added', () => {
const typeOld = new GraphQLObjectType({
name: 'Type',
fields: {
field1: { type: GraphQLString },
},
});
const typeNew = new GraphQLObjectType({
name: 'Type',
fields: {
field1: { type: GraphQLString },
field2: {
type: GraphQLString,
description: 'Something rather',
},
},
});

const oldSchema = new GraphQLSchema({
query: queryType,
types: [typeOld],
});
const newSchema = new GraphQLSchema({
query: queryType,
types: [typeNew],
});
expect(findDescriptionChanges(oldSchema, newSchema)[0].description).to.eql(
'New FIELD field2 added with description.',
);
expect(findDescriptionChanges(oldSchema, oldSchema)).to.eql([]);
expect(findDescriptionChanges(newSchema, newSchema)).to.eql([]);
});
});
163 changes: 163 additions & 0 deletions src/utilities/findDescriptionChanges.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/* eslint-disable no-restricted-syntax */
Copy link
Member

Choose a reason for hiding this comment

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

Please remove and fix all eslint errors if any.

// @flow
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same header license + @flow strict as other files


import {
GraphQLInterfaceType,
GraphQLObjectType,
GraphQLEnumType,
GraphQLInputObjectType,
} from '../type/definition';
import type { GraphQLFieldMap } from '../type/definition';
import { GraphQLSchema } from '../type/schema';
import invariant from '../jsutils/invariant';

export const DescribedObjectType = {
FIELD: 'FIELD',
TYPE: 'TYPE',
ARGUMENT: 'ARGUMENT',
ENUM_VALUE: 'ENUM_VALUE',
Copy link
Member

Choose a reason for hiding this comment

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

Your missing directives + you need to check arguments of directives in findDescriptionChanges

};

export const DescriptionChangeType = {
OBJECT_ADDED: 'OBJECT_ADDED',
Copy link
Member

Choose a reason for hiding this comment

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

As I understand it trigger when any new type is added not only GraphQLObjectType. So I think it should have a different name.

DESCRIPTION_ADDED: 'DESCRIPTION_ADDED',
DESCRIPTION_CHANGED: 'DESCRIPTION_CHANGED',
};

export type DescriptionChange = {
object: $Keys<typeof DescribedObjectType>,
change: $Keys<typeof DescriptionChangeType>,
description: string,
oldThing: any,
newThing: any,
};

/**
* Given two schemas, returns an Array containing descriptions of any
* descriptions that are new or changed and need review.
*/
export function findDescriptionChanges(
oldSchema: GraphQLSchema,
newSchema: GraphQLSchema,
): Array<DescriptionChange> {
const oldTypeMap = oldSchema.getTypeMap();
const newTypeMap = newSchema.getTypeMap();

const descriptionChanges: Array<?DescriptionChange> = [];

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

descriptionChanges.push(
generateDescriptionChange(newType, oldType, DescribedObjectType.TYPE),
);

if (
newType instanceof GraphQLObjectType ||
newType instanceof GraphQLInterfaceType ||
newType instanceof GraphQLInputObjectType
) {
invariant(
!oldType ||
oldType instanceof GraphQLObjectType ||
oldType instanceof GraphQLInterfaceType ||
oldType instanceof GraphQLInputObjectType,
'Expected oldType to also have fields',
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be an error.
For example:

scalar Date
type Date {
  day: Int
  month: Int
  year: Int
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This was added to satisfy Flow (#1127 (comment)).

This (and the enum case) are breaking schema changes, so an error seemed appropriate? Would you prefer they were silently ignored? If so, how do I do that in a way that Flow will recognize?

Copy link
Member

@IvanGoncharov IvanGoncharov Apr 2, 2018

Choose a reason for hiding this comment

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

So if I do breaking change in my schema I don't need to notify my documentation team about other non-breaking changes.
BTW Breaking changes is normal in many cases for example if you have your API in public beta.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. I was thinking too much about our specific case (where breaking changes already ping a substantial set of people for review and the docs changes are less relevant) but in the general case this should absolutely work.

I'm still not sure how to satisfy Flow without an invariant, but that's purely me not being much of a javascript developer.

);
const oldTypeFields: ?GraphQLFieldMap<*, *> = oldType
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this differentiate between the old type not existing vs the old type existing but the field not existing?

It could be overwhelming to see a ton of "Description added on new field" for every field on a type that was newly added, that might not be helpful.

Instead perhaps "New type added with description" and "New field added with description" should be different from "Description added on field"

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be overwhelming to see a ton of "Description added on new field" for every field on a type that was newly added, that might not be helpful.

The goal was to provide our docs team with a comprehensive set of all the new strings that needed reviewing; I'll type these differently when I add stronger types per your earlier comment, and then the caller can filter as desired.

? oldType.getFields()
: null;
const newTypeFields: GraphQLFieldMap<*, *> = newType.getFields();

Object.keys(newTypeFields).forEach(fieldName => {
const oldField = oldTypeFields ? oldTypeFields[fieldName] : null;
const newField = newTypeFields[fieldName];

descriptionChanges.push(
generateDescriptionChange(
newField,
oldField,
DescribedObjectType.FIELD,
),
);

if (!newField.args) {
return;
}

newField.args.forEach(newArg => {
const oldArg = oldField
? oldField.args.find(arg => arg.name === newArg.name)
: null;

descriptionChanges.push(
generateDescriptionChange(
newArg,
oldArg,
DescribedObjectType.ARGUMENT,
),
);
});
});
} else if (newType instanceof GraphQLEnumType) {
invariant(
!oldType || oldType instanceof GraphQLEnumType,
'Expected oldType to also have values',
);
Copy link
Member

Choose a reason for hiding this comment

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

Same here:

scalar Month
enum Month {
  Jan
  Feb
  # ...
}

const oldValues = oldType ? oldType.getValues() : null;
const newValues = newType.getValues();
newValues.forEach(newValue => {
const oldValue = oldValues
? oldValues.find(value => value.name === newValue.name)
: null;

descriptionChanges.push(
generateDescriptionChange(
newValue,
oldValue,
DescribedObjectType.ENUM_VALUE,
),
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Much of the logic above looks repetitive, with the exception of the strings referencing each kind of type. Could you factor out the common logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look. Javascript isn't my forte so for a first pass I definitely stuck with a simpler more explicit style.

}
});

return descriptionChanges.filter(Boolean);
}

function generateDescriptionChange(
newThing,
oldThing,
objectType: $Keys<typeof DescribedObjectType>,
): ?DescriptionChange {
if (!newThing.description) {
return;
}

if (!oldThing) {
return {
object: objectType,
change: DescriptionChangeType.OBJECT_ADDED,
oldThing,
newThing,
description: `New ${objectType} ${newThing.name} added with description.`,
};
} else if (!oldThing.description) {
return {
object: objectType,
change: DescriptionChangeType.DESCRIPTION_ADDED,
oldThing,
newThing,
description: `Description added on ${objectType} ${newThing.name}.`,
};
} else if (oldThing.description !== newThing.description) {
return {
object: objectType,
change: DescriptionChangeType.DESCRIPTION_CHANGED,
oldThing,
newThing,
description: `Description changed on ${objectType} ${newThing.name}.`,
};
}
}