#1341 findBreakingChanges to return line and column of the change

[Carglass]

linked to issue #1341

I saw that this issue has not received new pull requests for a while, or at least I dont think so. I am proposing an implementation to answer the feature request from issue #1341. So far, I have added fields in the Breaking Change types to give the locations of the change in the old schema and the new schema. they will be left empty if the schema was not generated from SDL. I also updated the tests so that they pass with the lines and columns for change added

I wanted to ask if the direction is good. More specifically, I felt like returning this in breaking changes could be an option, as well as returning the source schema strings (without which it is hard to find the location when the schema is declared inside a js file, for now I have not yet added it to BreakingChange).

For now, only findRemovedTypes is implemented, I will continue working on it if that fits the need,

Thanks for your comments

[IvanGoncharov]

@Carglass Thanks for PR 👍

I wanted to ask if the direction is good. More specifically, I felt like returning this in breaking changes could be an option, as well as returning the source schema strings (without which it is hard to find the location when the schema is declared inside a js file, for now, I have not yet added it to BreakingChange).

I think it's better to just return whole astNode since in current form you lose some important info:

graphql-js/src/language/source.js

Lines 27 to 29 in e7c1fde

	body: string;
	name: string;
	locationOffset: Location;

If a user needs to map astNode to location this function can be used:
https://github.com/graphql/graphql-js/blob/master/src/language/location.js#L20-L24

Also, note that it should be an array of nodes since one type can be defined in multiple documents:

let schema = buildSchema('type Query');
schema = extendSchema(schema, parse('extend type Query { foo: String }');
schema = extendSchema(schema, parse('extend type Query { bar: String }');

astNode will contain a first node:

graphql-js/src/type/definition.js

Line 669 in e7c1fde

astNode: ?ObjectTypeDefinitionNode;

and extensionASTNodes will contain another two:

graphql-js/src/type/definition.js

Line 670 in e7c1fde

extensionASTNodes: ?$ReadOnlyArray<ObjectTypeExtensionNode>;

So I need to think about what would be the best API design in this case.
Please feel free to ping me if I don't respond in the next few days.

[Carglass]

@IvanGoncharov Thanks for your detailed comments.

I think it's better to just return whole astNode since in current form you lose some important info:

I have started working on adding astNodes instead of only lines and columns. So far, my main issue is that I have had to refactor the tests to assert each node one by one ( I cannot include the whole astNode in the deep equal test). I will ping you once I have added it everywhere.

I will also wait for your input before working on returning an arrays including the potential extensions

[IvanGoncharov]

@Carglass Thanks for rebasing your changes on master.
Since I'm not the original author of this code I'm focusing on "Exploratory Refactoring" at the moment to better understand how this code is working and how to better extend it to support exposing AST nodes.
I already have a few ideas but I want to finish refactoring and test them on a small scale.
Thanks for patience I will try to finish it in the next few days.

[Carglass]

@Carglass Thanks for rebasing your changes on master.
Since I'm not the original author of this code I'm focusing on "Exploratory Refactoring" at the moment to better understand how this code is working and how to better extend it to support exposing AST nodes.
I already have a few ideas but I want to finish refactoring and test them on a small scale.
Thanks for patience I will try to finish it in the next few days.

@IvanGoncharov No problem on my side to rebase, please let me know when you are done with your changes. Regarding this issue in itself, I am not happy with my tests so far, I would ideally need to be able to do snapshots to follow the style of the current tests. Do you think we could add a chai snapshot library for example? The issue is that the astNodes are usually big objects, alternatively I could serialize them into files manually, but using a snapshot utility seems more convenient and reusable in case of later changes

[IvanGoncharov]

You don't need snapshot here just check that node inside the change is the same as in in-memory schema:

expect(findBreakingChanges(oldSchema, newSchema)).to.deep.equal([
  {
     type: BreakingChangeType.TYPE_REMOVED,
     description: 'Type1 was removed.',
     oldNode: oldSchema.getType('Type1').astNode,
     newNode: undefined,
  },
]);
``

[Carglass]

Thank you, that is a smart way to solve my issue indeed. I will rework those tests this way!

[IvanGoncharov]

No problem on my side to rebase, please let me know when you are done with your changes.

@Carglass Thanks for the patience. I finished all the major refactoring that can be done without changing public API.
I want to try a few alternative approaches to this PR in parallel but it shouldn't block you.

Regarding this issue in itself, I am not happy with my tests so far, I would ideally need to be able to do snapshots to follow the style of the current tests. Do you think we could add a chai snapshot library for example?

The main goal of this library is to be a reference implementation for other languages that's why we compromise the speed of development for code quality and better tests.
Problem with snapshot testing is that it's not fully reviewable and not a great thing for TDD in other languages (since snapshots are incompatible).
Snapshot testing is good for catching regressions or in case there some tests are better than no tests but in this project, both code and tests are intended to be reused.

The issue is that the astNodes are usually big objects, alternatively I could serialize them into files manually, but using a snapshot utility seems more convenient and reusable in case of later changes

Please see my comment above for how to solve this without relying on snapshots.

[Carglass]

@IvanGoncharov I have implemented the astNodes for all Breaking and DangerousChanges. For now, it is only the "regular" nodes and not the extension nodes. Please let me know about the direction regarding the API for the extension nodes. Also, please let me know of any comment or improvement on the changes so far :)

By the way, I rebased after your changes

[IvanGoncharov]

@Carglass I will try to take a look this weekends.
Plus still, need to figure out what to do with extensions nodes.