rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.36k stars 194 forks source link

Issue regarding null values #244

Closed nickydonna closed 5 years ago

nickydonna commented 6 years ago

We have an issue with an special merger. We need to keep the properties even if the source object has that property as undefined, so we use a custom merger, with almost all values they work ok, but when the original object has a value of null, the resulting property is set to undefined.

const Immutable = require("seamless-immutable").static;

const merger = (c, o) => {
    if (o === undefined && c !== undefined) {
      return c;
    }
    return;
}

Immutable.merge({a: 1}, {a: undefined}, {merger}) // {a: 1}
Immutable.merge({a: null}, {a: undefined}, {merger}) // {a: undefined}

Here is the code running on runkit https://runkit.com/donnanicolas/seamless-immutable-null

We don't know why this happens

crudh commented 6 years ago

Seems like a problem in the addToResult function in the merge function: https://github.com/rtfeldman/seamless-immutable/blob/281c344ad4694d4190fbe27d954d5a69887335bf/src/seamless-immutable.js#L405

So if the result from the merger is not truthy it will go to the else and use the value from o. It should do an explicit check for undefined instead. But I guess a fix for that would require a new major version since it can break stuff.

nickydonna commented 6 years ago

@crudh Ok, but it seem like a major issue, since anyone using null values can have issues like this and its rather hard to track

crudh commented 6 years ago

@donnanicolas yeah, I agree. PR #248 should fix it.

nickydonna commented 6 years ago

@crudh Thanks!! will update as soon as the PR is merged!! (I "reviewed" it and it seems great, but I don't know much about the code)

crudh commented 5 years ago

This is fixed now, a release will be done soon.