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

Don't merge if value is an empty string #178

Closed
TidyIQ opened this issue Oct 30, 2019 · 7 comments
Closed

Don't merge if value is an empty string #178

TidyIQ opened this issue Oct 30, 2019 · 7 comments

Comments

@TidyIQ
Copy link

TidyIQ commented Oct 30, 2019

I need to merge some data that originates from a .yaml file and is converted to a JS object using GraphQL (I'm using Gatsby with Netlify CMS if you need a real world use case).

As you may know, yaml does not allow undefined values. The closest alternative is to set the value to null or an empty string ('').

For example, let's say I have the following GraphQL query:

query MyQuery {
  foo {
    one
    two
    three
  }
}

Each field (one, two and three) are optional fields.

For example, if only two field has a value then the yaml file will be:

foo:
  two: Some text

If I attempt to run the query on that data, GraphQL will throw an error as fields one and three don't exist.

The best I can do is to set a default value for all fields as '' (an empty string). I can't set it to null due to an issue where users who change the value from the default null value, then delete the value, will save the value as an empty string.

For example, if only the two field has a value then the yaml file will be:

foo:
  one: ''
  two: New text
  three: ''

And after running the GraphQL query, it will return the following object:

{
  foo: {
    one: "",
    two: "New text",
    three: ""
  }
}

If I wanted to merge it with the following object:

{
  foo: {
    one: "Some default text",
    two: "Some other default text",
    three: "Even more default text"
  }
}

Then run deepmerge() on the two objects, it will simply return the first object:

{
  foo: {
    one: "",
    two: "New text",
    three: ""
  }
}

Having an option in deepmerge to ignore empty strings would be a lifesaver for this, for example:

const newObject = {
  foo: {
    one: "",
    two: "New text",
    three: ""
  }
}

const defaultObject = {
  foo: {
    one: "Some default text",
    two: "Some other default text",
    three: "Even more default text"
  }
}

const mergedObject = deepmerge(newObject, defaultObject, ignoreEmptyStrings);

Would result in mergedObject being equal to:

{
  foo: {
    one: "Some default text",
    two: "New  text",
    three: "Even more default text"
  }
}
@TehShrike
Copy link
Owner

That is the current behavior: https://codepen.io/TehShrike/pen/Jjjrwzo

What version of deepmerge are you using? A bug with falsey values not being handled properly was recently fixed in #172

@TidyIQ
Copy link
Author

TidyIQ commented Oct 31, 2019

Ah ok. I'm using 4.0 but I see 4.2 is available now. I'll try upgrading and see if it fixes it. Thanks!

@johnlife
Copy link

johnlife commented Dec 22, 2019

That is the current behavior: https://codepen.io/TehShrike/pen/Jjjrwzo

What version of deepmerge are you using? A bug with falsey values not being handled properly was recently fixed in #172

In your codepen with 4.2.2 version neither deepmerge(newObject, defaultObject) nor deepmerge(defaultObject, newObject) produce

{
  foo: {
    one: "Some default text",
    two: "New  text",
    three: "Even more default text"
  }
}

Why is this closed? Can we have an option to specify values to treat as non-existent?

@TehShrike
Copy link
Owner

I see. You had the arguments backwards in your original post – it sounds like you want deepmerge(defaultObject, newObject).

merge(x, y, [options])
If an element at the same key is present for both x and y, the value from y will appear in the result.
the readme

Unfortunately, I don't believe there's a way to twist isMergeableObject + customMerge to fit your use case, since isMergeableObject gets called on the target first, and then the target's keys get iterated over before the custom merge function gets called.

There have been some other edge case requests like this that have come up. I'm tempted to alter the customMerge function signature so that the first function would get passed both values as well as just the key, so you could do any wacky pre-merge logic in that function as well. That would allow you to do this (which isn't currently possible, but I think would solve your use case):

const mergedObject = deepmerge(defaultObject, newObject, {
  customMerge(key, a, b) {
    if (a === '') return () => b
  },
})

@johnlife
Copy link

johnlife commented Dec 22, 2019

Nah, it' not my original post, I'm just fighting with the same issue now. For me API returns nulls and undefineds, that should not replace the default values during merge.

This can be fixed with replacing

  if (propertyIsOnObject(target, key) && options.isMergeableObject(source[key])) {
    destination[key] = getMergeFunction(key, options)(target[key], source[key], options)
  } else {

with

  if (propertyIsOnObject(target, key)) {
    if (options.isEmpty && options.isEmpty(source[key])) {
      destination[key] = cloneUnlessOtherwiseSpecified(target[key], options)
    } else if (options.isMergeableObject(source[key])) {
      destination[key] = getMergeFunction(key, options)(target[key], source[key], options)
    } else {
	destination[key] = cloneUnlessOtherwiseSpecified(source[key], options)
    }
  } else {

Then the following code

const newObject = {
  foo: {
    one: null,
    two: "New text",
    three: ""
  }
}

const defaultObject = {
  foo: {
    one: "Some default text",
    two: "Some other default text",
    three: "Even more default text"
  }
}

const mergedObject = deepmerge(defaultObject, newObject, {
  isEmpty: a => a === null || a === '',
});
console.log(mergedObject);

gives you

{ 
  foo: { 
    one: 'Some default text',
     two: 'New text',
     three: 'Even more default text' 
  } 
}

PR Created:
#185

@johnlife
Copy link

const mergedObject = deepmerge(defaultObject, newObject, {
  customMerge(key, a, b) {
    if (a === '') return () => b
  },
})

I think this requires deep understanding of an internal code of a library and I'd go with more human-readable options, but you're maintainer.

But shouldn't your code be

const mergedObject = deepmerge(defaultObject, newObject, {
  customMerge(key, a, b) {
    if (b === '' || b === null || b === undefined) return () => a
  },
})

Or is it somewhere swapped the order of parameters?

@n0v1
Copy link

n0v1 commented Apr 28, 2020

I'd favor the more generalized customMerge way to solve this because in my case I'd only want to ignore empty strings and undefined but keep null values. Others may have other requirements and customMerge(key, a, b) would solve all of them.

If this is documented in the readme with examples like posted in this issue, this shouldn't be too hard to use.

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

No branches or pull requests

4 participants