added the fixed for merge interface and scalar

[iamrommel]

This fixes the duplicates for scalar and interface from different files.

I just added the Kind.SCALAR_TYPE_DEFINITION and Kind.INTERFACE_TYPE_DEFINITION from the condition src/utilities/astHelpers.js (line 8 & 9) to be included for objectType definition.

Ow, i just notice now that i updated the package.json too by adding the cross-env so the scripts will work even on my windows OS, which i guess fine :)

[cfnelson]

@iamrommel Thanks for this PR! It's greatly appreciated!

Would you be able to create a few tests to demonstrate the behaviour of the changes you have made. You can check the tests folder for examples of existing tests. As it's not currently clear what this PR will be fixing? Maybe you could update your PR to explain what this is fixing/doing? As we will need to update the changeLog as well to reference this fix & changes.

Is this PR related to this issue #158 ?
I also see that this issue here would be resolved by your changes: #141 was that the intent of this PR?

[iamrommel]

The PR does not break any existing test so i did not bother to create a new one.. :)

But this fixes the issue for merging duplicates scalar and interface of graphql, which is related to #141 which i guess will fix the issue at #158 too

I will create a test as soon as i get back from my 1 week holiday, but if you could help me out for test while i'm away and publish it, i think it will be beneficial for other guys that are waiting for a fix 4months ago. As of now, i forked it and used my released version from https://github.com/iamrommel/merge-graphql-schemas/releases/tag/v1.5.4

[iamrommel]

@cfnelson , test are added and everything is still success, let me know what are other things to do before this is merged and publish the updates

[cfnelson]

@iamrommel Thanks for the extra tests! We just need to update the changeLog with reference to this PR and what was fixed. We might wish to batch this release in with some pkg.json updates, and potentially the other open PR, though we won't hold this up if it's not ready.

Will go through this PR with the other maintainer tomorrow, if all is good with him, we will hopefully release it sometime tomorrow. 👍

[iamrommel]

that is awesome, i can wait though as i'm using my forked version on my app.

BTW, I notice that there are lot of changes on test file, because of formatting, I was using webstorm and windows and some long lines of code was formatted to have it next line. I think that was defined too on the .eslint config

[cfnelson]

@iamrommel No worries, for some reason I thought this pkg had prettier and a format command but it looks like we only have eslint. You could try to run the lintfix script and see if that will format it correctly. Otherwise no worries, maybe we can add prettier so formatting can be more consistent.

If you want to update the changelog in this PR that would be great, otherwise as long as we have push access on this PR we can update the changelog tomorrow once we take a look.

I noticed that this PR's tests only cover merging enums with the exact same fieldDefs.

enum ClientStatus {
    NEW
    ACTIVE
    INACTIVE
  }
enum ClientStatus {
    NEW
    ACTIVE
    INACTIVE
  }

What is the (expected) behaviour if you have ->

enum ClientStatus {
    NEW
    ACTIVE
    INACTIVE
  }
enum ClientStatus {
    ACTIVE
    INACTIVE
  }

Might be good to add a test similar to the above^. If there is already such a test and I missed it, my apologies.

Thanks again for the PR! We appreciate all the help we can get 😄

[iamrommel]

Ow... I don't have that scenario on my test, but i assumed that the last one will win on merging. BTW, what should really be the behavior, 1. Do i need to throw error? 2. Do i let the last one wins? 3. Or do i need to merge the enum items? I have those 3 options but what do you think it should be? *Rommel C. Manalo*

…

On Wed, Sep 12, 2018 at 9:15 AM Charles Nelson ***@***.***> wrote: @iamrommel <https://github.com/iamrommel> No worries, for some reason I thought this pkg had prettier and a format command but it looks like we only have eslint. You could try to run the lintfix script and see if that will format it correctly. Otherwise no worries, maybe we can add prettier so formatting can be more consistent. If you want to update the changelog in this PR that would be great, otherwise as long as we have push access on this PR we can update the changelog tomorrow once we take a look. I noticed that this PR's tests only cover merging enums with the exact same fieldDefs. enum ClientStatus { NEW ACTIVE INACTIVE } enum ClientStatus { NEW ACTIVE INACTIVE } What is the (expected) behaviour if you have -> enum ClientStatus { NEW ACTIVE INACTIVE } enum ClientStatus { ACTIVE INACTIVE } Might be good to add a test similar to the above^. If there is already such a test and I missed it, my apologies. Thanks again for the PR! We appreciate all the help we can get 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK0cU_-X-PgqGw29kp-aOsvQJ1PwhV1ks5uaGAkgaJpZM4WRjqD> .

[cfnelson]

@iamrommel I would go with what most people would expect to occur. Also this may not actually be throwing an error. The order could also effect the outcome, e.g flip the example I described around.

I would avoid point 2. as it will not always be clear to someone which definition is processed first/last, and could lead to subtle bugs.

The safest option would be point 1. throw and error and ask user to fix the enum definitions.

Personally if it's safe, I would take a guess that people would expect or want point 3. to merge the enum items together (I could be wrong though and option 1 is the preferred). I suspect people might expect enums to behave the same way as merging the fieldDefs on types when all: true is used.

We should follow the existing pattern of controlling the behaviour by passing the all: true option to mergeTypes(types, { all: true });. If all: true is used or a specific enum option we could do point 3., if not passed e.g defaults to false then we throw an error asking user to resolve the difference.

Does one of these scenarios sound more appealing/what you would expect? If your able to update this PR with a few tests to confirm the behaviour and or make changes to attempt the merge that would be amazing! 🙌

[iamrommel]

We should follow the existing pattern of controlling the behaviour by passing the all: true option to mergeTypes(types, { all: true });. If all: true is used or a specific enum option we could do point 3., if not passed e.g defaults to false then we throw an error asking user to resolve the difference.

This looks better for me. I'll update the PR asap

[iamrommel]

Thanks for merging and deploying @RodMachado
Thanks too @cfnelson for updating the test, sorry i was buried with some work last time that is why i was not able to fix it.

I have tested the 1.5.4 version and it looks good on my own project.

I have followed okgrow on medium and your works are awesome, so keep it up and cool guys. :)

[cfnelson]

Thanks again for contributing! We appreciate all the help we get on this pkg. 😄