Skip to content
This repository has been archived by the owner on Jan 23, 2021. It is now read-only.

property with null value in source object not overwriting to same property in target object #23

Closed
topgun743 opened this issue Dec 30, 2016 · 6 comments

Comments

@topgun743
Copy link

Hi Dev,
Following is not working correct:

var deepAssign = require("deep-assign");
deepAssign({}, {Lat: null, Lon: null}, {Lat: null, Lon: 2})

output is:
{Lon: 2}

It must be:
{Lat: null, Lon 2}

Due to this weird issue, my form validation is failing as Lat property is totally missing from output.
If I use Object.assign, it works correctly.
Can you plz guide me what is wrong?

@schnittstabil
Copy link
Contributor

schnittstabil commented Dec 30, 2016

Unfortunately, this's on purpose. From MDN:

… null and undefined will be ignored.

More precisely, this is due to the ECMAScript® 2015 Language Specification; 19.1.2.1 Object.assign:

5.a If nextSource is undefined or null, let keys be an empty List.

As deep-assign is Recursive Object.assign(), the following happens:

deepAssign({}, {Lat: null,}, {Lat: null,})

// should be similar to
{Lat: deepAssign(<?>, null, null), …}

// which have to be the same as 
{Lat: Object.assign(<?>, null, null), …}

The <?> and the exact recursive semantic is debatable in this case. deep-assign ignores the <?> at the moment – to be at least a bit useful. But in all possible interpretations, the (additional) null values must be ignored because of the spec.

Hence, I believe deep-assign does not match your requirements. There are many merge tools doing this in a different way. Some examples:

@topgun743
Copy link
Author

This is strange behavior suggested by ECMA. Null may b a valid and intended value from source. How come not accept it? Also deepAssign must provide an option to even allow such kind of merging.

@schnittstabil
Copy link
Contributor

It would be nice to provide options somehow, but the signature is:

deepAssign(target, source, [source, ...])

If you have an idea, which does not conflict with the semantics of Object.assign, then please let us know.

Btw, we must not provide an option – we are free to do; and we are not ECMA of course 😉. deep-assign is fully dedicated being Recursive Object.assign(); therefore we can also create a new project with a different semantic – like, I did with merge-options.

@topgun743 Anyway, does neither _.merge nor merge-options provide you the semantic you need?

@topgun743
Copy link
Author

Looks like I have to turn towards your suggestion. Yes, merge-options looks nice. Will use it now off course. Thanks for suggesting. Great !

@topgun743
Copy link
Author

BTW, you can allow a last object as an option config sort of to deepAssign whose first property can be like

{allowNulls: true, . . . . . . other options}

This way it would be assumed that even null values from source should be forced onto the target. I think it should work.

@schnittstabil
Copy link
Contributor

Sadly, because of the Object.assign signature/semantics, these options must be also assigned to the result object, which would be undesirable in my opinion.

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

No branches or pull requests

2 participants