Replace 'find{Breaking,Dangerous}Changes' with generic 'findSchemaChanges'

[wtrocki]

Motivation

Currently to find potentially Breaking and Dangerous changes developers need to call 2 separate methods and execute logic twice. This change simply exposes the underlying method that will give access to both with single execution logic.

[IvanGoncharov]

@wtrocki Totally reasonable 👍
Moreover, I think it's confusing to have multiple functions that basically do the same so
I think findSchemaChanges should be a function to rule them all.

Since we working on new major release 15.0.0 I think its good time to remove findDangerousChanges and findBreakingChanges. And merge BreakingChange | DangerousChange into single SchemaChange type.

Can you please do that?

Also, don't forget to update TS typings in tstypes folder.
Another thing all public APIs should be exported through src/index.js and src/utilities/index.js.
https://github.com/graphql/graphql-js/blob/master/src/README.md

[IvanGoncharov]

It would also allow merging #1127 by @eapache without adding new API functions.

[wtrocki]

Working on this now

[wtrocki]

What is the policy on documenting breaking changes/providing migration paths?

[IvanGoncharov]

@wtrocki These functions are used mostly in the build scripts and CLI tools so migration path should be pretty easy. We have a script that generates changelog that will list all breaking changes. Would be great if you write a small note about this change and how to implement findBreakingChange/findDangerousChange on top of findSchemaChange and after release, I will manually add this note into the generated changelog.

[IvanGoncharov]

It's better to create common SchemaChangeType:

export const SchemaChangeType = Object.freeze({
  ...BreakingChangeType,
  ... DangerousChangeType,
});

[wtrocki]

Perfect. I was thinking about it

[IvanGoncharov]

@wtrocki Since you effectively merging tests for findBreakingChanges and findDangerousChanges please adjust tests accordingly.
For example, rename this one to should detect if a field is added to an input type and remove the duplicating test:

graphql-js/src/utilities/__tests__/findBreakingChanges-test.js

Lines 1026 to 1047 in 06ef5db

	it('should detect if an optional field was added to an input', () => {
	const oldSchema = buildSchema(`
	input InputType1 {
	field1: String
	}
	`);
	const newSchema = buildSchema(`
	input InputType1 {
	field1: String
	field2: Int
	}
	`);
	expect(findDangerousChanges(oldSchema, newSchema)).to.deep.equal([
	{
	type: DangerousChangeType.OPTIONAL_INPUT_FIELD_ADDED,
	description:
	'An optional field field2 on input type InputType1 was added.',
	},
	]);
	});

[IvanGoncharov]

@wtrocki Please do the same for other tests where you modify snapshot.

[mjmahone]

Leaving comment on commit, as I disagree a bit with this premise, I think.

[mjmahone]

I want to add a note of warning: the purpose of "findBreakingChanges" is to find only those changes to the schema that could potentially break old versions of clients that are shipped and can never be updated.

A function called findSchemaChanges needs to have different behavior: it needs to say whether a type was added or removed, whether there's a new field, etc.

This is OK, and even desireable to have. But I believe we should keep findDangerousChanges, or have a DANGEROUS_CHANGES const that we can use with findSchemaChanges to preserve the functionality of identifying only those changes that will cause backwards-compatibility issues with existing, in production applications. Also something to note: the "potentially dangerous" changes subset basically never causes us problems, at least at Facebook. We build our applications and our infrastructure surrounding GraphQL in a way where they do not cause breakages, though if you get it wrong (for instance, using exhaustive switch statements with enums), these class of changes will cause problems.

If the intention continues to be finding only those changes that may cause problems, we should have findSchemaChanges act more like our validation rules: you can pass in a list of changes to find (with the "dangerous" set, or the "known to be breaking" set, or "all" set). This is something I'd heartily support. But again, if we name it findSchemaChanges, it needs to be able to find all schema changes, not just those that might be breakages.

[IvanGoncharov]

This is OK, and even desirable to have. But I believe we should keep findDangerousChanges, or have a DANGEROUS_CHANGES const that we can use with findSchemaChanges to preserve the functionality

@mjmahone Thanks for review 👍 It's exactly the plan since we already expose constants for both breaking and dangerous changes:

graphql-js/src/utilities/findBreakingChanges.js

Lines 38 to 63 in 06ef5db

	export const BreakingChangeType = Object.freeze({
	TYPE_REMOVED: 'TYPE_REMOVED',
	TYPE_CHANGED_KIND: 'TYPE_CHANGED_KIND',
	TYPE_REMOVED_FROM_UNION: 'TYPE_REMOVED_FROM_UNION',
	VALUE_REMOVED_FROM_ENUM: 'VALUE_REMOVED_FROM_ENUM',
	REQUIRED_INPUT_FIELD_ADDED: 'REQUIRED_INPUT_FIELD_ADDED',
	INTERFACE_REMOVED_FROM_OBJECT: 'INTERFACE_REMOVED_FROM_OBJECT',
	FIELD_REMOVED: 'FIELD_REMOVED',
	FIELD_CHANGED_KIND: 'FIELD_CHANGED_KIND',
	REQUIRED_ARG_ADDED: 'REQUIRED_ARG_ADDED',
	ARG_REMOVED: 'ARG_REMOVED',
	ARG_CHANGED_KIND: 'ARG_CHANGED_KIND',
	DIRECTIVE_REMOVED: 'DIRECTIVE_REMOVED',
	DIRECTIVE_ARG_REMOVED: 'DIRECTIVE_ARG_REMOVED',
	REQUIRED_DIRECTIVE_ARG_ADDED: 'REQUIRED_DIRECTIVE_ARG_ADDED',
	DIRECTIVE_LOCATION_REMOVED: 'DIRECTIVE_LOCATION_REMOVED',
	});
	export const DangerousChangeType = Object.freeze({
	VALUE_ADDED_TO_ENUM: 'VALUE_ADDED_TO_ENUM',
	TYPE_ADDED_TO_UNION: 'TYPE_ADDED_TO_UNION',
	OPTIONAL_INPUT_FIELD_ADDED: 'OPTIONAL_INPUT_FIELD_ADDED',
	OPTIONAL_ARG_ADDED: 'OPTIONAL_ARG_ADDED',
	INTERFACE_ADDED_TO_OBJECT: 'INTERFACE_ADDED_TO_OBJECT',
	ARG_DEFAULT_VALUE_CHANGE: 'ARG_DEFAULT_VALUE_CHANGE',
	});

So it's just a matter of writing one-liner like this:

import { BreakingChangeType, findSchemaChanges } from 'graphql';

function findBreakingChanges(oldSchema, newSchema) {
  return findSchemaChanges(oldSchema, newSchema)
    .filter(({type}) => type in BreakingChangeType);
}

Also both constant is already part of public API and exposed through main index.js:

graphql-js/src/index.js

Lines 403 to 405 in 06ef5db

	// Compares two GraphQLSchemas and detects breaking changes.
	BreakingChangeType,
	DangerousChangeType,

And there is no plans to ever remove those constants.

@mjmahone @Neitsch Does this migration path address your concerns?

[wtrocki]

The awesome point from @mjmahone
To extend it, there are two use cases:

1. Comparing changes and see what was added/removed
   (with metadata about what and where)
2. Finding Breaking and Dangerous Changes
   (current implementation in graphql-js)

My initial intention was to expose a single function that can be used to perform option nr 2 by calling a single function, so people can get both at the same time with a single method call and then decide on their own what they define as dangerous etc.

As far I checked both enums do not comprehensively represent all possible changes that can happen so option nr1 will require separate method anyway. I have done some research how such implementation could look like and found out that there is already a community package that does both:
https://github.com/kamilkisiela/graphql-inspector

With a generic change interface:

https://github.com/kamilkisiela/graphql-inspector/blob/33ca4f9f37c5b6bedba6b6ef190440777050cc9c/packages/core/src/diff/changes/change.ts#L1-L59

With all of that in mind using findSchemaChanges for case nr2 might be confusing as developers might read the intention of this method as a solution for case nr1. Maybe we can simply rename it to state intentions properly. Do you think it will be useful to have case nr 1 implemented? If

If the intention continues to be finding only those changes that may cause problems, we should have findSchemaChanges act more like our validation rules: you can pass in a list of changes to find (with the "dangerous" set, or the "known to be breaking" set, or "all" set). This is something I'd heartily support

WDYT about:

1. having a generic way to find all changes and then still have these two methods that will be able to categorize them towards Dangerous/Breaking

function findBreakingChanges(oldSchema, newSchema) {
  return findSchemaChanges(oldSchema, newSchema)
    .filter(({type}) => type in BreakingChangeType);
}

2. having a generic way to find all changes and remove those methods. The generic function could accept list of the changes
   (This gives much better flexibility but it will be more complex to implement)

  return findSchemaChanges(oldSchema, newSchema, {types: BeakingChangeTypes})

[IvanGoncharov]

@wtrocki So the idea is that we need to merge other types of schema changes in future, e.g. #1127. Classifying all changes into non-overlapping categories can be tricky so instead, we can simply do:

export const SchemaChangeType = Object.freeze({
  ...BreakingChangeType,
  ... DangerousChangeType,
  TYPE_ADDED: 'TYPE_ADDED',
  // All other type of changes
});

return findSchemaChanges(oldSchema, newSchema, {types: BeakingChangeTypes})

For very big schemas it preferable to get all changes in single iteration and it will look awkward:

const changes = findSchemaChanges(oldSchema, newSchema, {types: {
   ...BeakingChangeTypes,
   ...DangerousChangeTypes,
}})
for (const change of changes) {
  if (change in BreakingChangeTypes) {
    reportError(change);
  } else if (change in DangerousChangeTypes) {
    reportWarning(change);
  }
}

[wtrocki]

I will wait for consensus and then apply the required changes.

[mjmahone]

I'm a big fan of this having an almost identical API as validate, i.e. (original: GraphQLSchema, updated: GraphQLSchema, findChanges: $ReadOnlyArray<FindChangeFn> = ALL_SCHEMA_CHANGES). Though if we went that far, it might make sense to have it operate over an SDL instead of over a GraphQLSchema. We could always have findSchemaChanges be an internal function to start, and re-implement findBreakingChanges and findDangerousChanges using findSchemaChanges.

In short: I'm happy with adding a findSchemaChanges function that can be used in the future to find all schema changes. I think we should definitely push this new function into the repo, even if it's only internal, so it's easier to iterate on. I don't have strong opinions on what exactly the API is, but I do think it should have the business logic for finding changes, and at least allow you the flexibility to choose whether you want to find all changes, breaking-only changes, or dangerous-only changes. Having a solid API that provides even more flexibility would be even more desireable, but we don't, IMO, need to block getting something in until we have that ideal API.

[mjmahone]

A lot of tooling currently relies on only getting the schema changes. I think all the code changes from above are great, but until we make findSchemaChanges more flexible, we should keep these utility functions, to avoid forcing people to rewrite their code significantly in order to upgrade.

[IvanGoncharov]

I'm happy with adding a findSchemaChanges function that can be used in the future to find all schema changes. I think we should definitely push this new function into the repo, even if it's only internal, so it's easier to iterate on. I don't have strong opinions on what exactly the API is, but I do think it should have the business logic for finding changes, and at least allow you the flexibility to choose whether you want to find all changes, breaking-only changes, or dangerous-only changes.

@mjmahone So just to clarify you oppose exposing findSchemaChanges in its current form?
(This is a core change in this PR, all the rest is just consequences of removing findBreakingChanges and findDangerousChanges).

Personally I think there is no big difference in filtering after findSchemaChanges and splitting change detection into separate functions will increase significantly code size/complexity and significantly slow down change detection on big schemas(since you need to do whole schema iteration in every rule).

But I'm totally ok with an alternative implementation if code size/performance will stay the same.

[JoviDeCroock]

Implemented in #4235