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

mergeDeep behaviour with undefined values changed in 10.6.2 #7011

Open
4 tasks
angrykoala opened this issue Mar 7, 2025 · 5 comments · May be fixed by #7012
Open
4 tasks

mergeDeep behaviour with undefined values changed in 10.6.2 #7011

angrykoala opened this issue Mar 7, 2025 · 5 comments · May be fixed by #7012

Comments

@angrykoala
Copy link

Issue workflow progress

Progress of the issue based on the
Contributor Workflow

  • 1. The issue provides a reproduction available on Github, Stackblitz or CodeSandbox

    Make sure to fork this template and run yarn generate in the terminal.

    Please make sure the GraphQL Tools package versions under package.json matches yours.

  • 2. A failing test has been provided
  • 3. A local solution has been provided
  • 4. A pull request is pending review

Describe the bug

In version 10.6.1 of @graphql-tools/utils, undefined values are ignored from mergeDeep. For example:

mergeDeep([{a:"dsa"}, {a:"dd", b: 1}, undefined])

Returns { a: 'dd', b: 1 }

In version 10.6.2 and beyond, the same input returns undefined

To Reproduce Steps to reproduce the behavior:

In version 10.6.2:

mergeDeep([{a:"dsa"}, {a:"dd", b: 1}, undefined])

Returns undefined

Expected behavior

To return the same as 10.6.1:

{ a: 'dd', b: 1 }
@angrykoala
Copy link
Author

angrykoala commented Mar 7, 2025

In addition to the aforementioned problem the following inputs are also different between 10.6.1 and 10.6.2

Input 10.6.1 10.6.2
mergeDeep([undefined]) {} undefined
mergeDeep([]) {} []
mergeDeep([{}]) {} undefined

@ardatan ardatan linked a pull request Mar 7, 2025 that will close this issue
@ardatan
Copy link
Owner

ardatan commented Mar 7, 2025

Thanks for the feedback!
I created the PR for fixing the behavior!
#7012

@angrykoala
Copy link
Author

Thanks for the quick turnaround.

One question: per the PR I see that some cases (e.g. "no sources should return undefined") are not really what I expected, that behavior is different to how it was in 10.6.1 (always returning an empty object) and I expected a change like that to be considered a breaking change (i.e. released in a major release)

Here I assuming graphql-tools follow semantic versioning or a similar versioning

Thanks

@ardatan
Copy link
Owner

ardatan commented Mar 7, 2025

We follow semantic versioning, right.

The behavior was broken and not intended as you can see in the tests. There is no change in the API but the result is corrected in the PR. So it is a bug fix not a major breaking change in my opinion.
mergeDeep takes an array of values to be merged. So if there is no value in the passed array, it should be undefined not an empty object.
If an array with undefined values passed, nullish values should be ignored like you expected in the original issue, and the result should be undefined.
I am not sure if relying on a broken result is a good idea.
Let me know if I am missing something here.

@angrykoala
Copy link
Author

Well, that behavior has been how this tool has been working for a long time if I'm not mistaken, and as this particular case was not documented afaik, we assumed that to be the expected behavior (always return an object) and relied on that for our tool as we don't have any checks for the result after mergeDeep

I agree that relying on unintended behavior is not ideal, and we are not massively affected by this as we can always add some extra checks on our end before updating, but wanted to warn about the potential breaking of other tools by this fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants