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

Issue regarding null values #244

Closed
nickydonna opened this issue Mar 6, 2018 · 5 comments · Fixed by #248
Closed

Issue regarding null values #244

nickydonna opened this issue Mar 6, 2018 · 5 comments · Fixed by #248

Comments

@nickydonna
Copy link

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
Copy link
Collaborator

crudh commented Mar 23, 2018

Seems like a problem in the addToResult function in the merge function:

if (mergerResult) {

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
Copy link
Author

@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
Copy link
Collaborator

crudh commented Mar 26, 2018

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

@nickydonna
Copy link
Author

@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
Copy link
Collaborator

crudh commented Aug 31, 2018

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

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