-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
V4 get in fix #1362
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
V4 get in fix #1362
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigating this.
However, this isn't quite right since it changes the behavior of getIn()
in a breaking way - there is a significant difference between null | undefined
and NOT_SET
.
I think the important detail is whether the value is internal along the key path or the final value. I think you want to be altering line 851 instead
@leebyron I think I'm struggling to understand how to fix this then. Take this example: const collection = Immutable.fromJS({a: {b: null}});
// ---------------------------------v [ value is null]
console.log(collection.getIn(['a', 'b', 'c'], 'notSetValue'); // should return string: 'notSetValue' As I understand it, the fix for this issue (#1361) is to make it so that when value is falsy or value doesn't have an internal get method BUT that value is null or undefined we want to return the If so, the only other fix I can think of here is introducing a new conditional, above line 851 that says: if (value === null || value === undefined) {
return notSetValue;
} However, this fix seems to have the same result as the current change in this PR In Immutable v3, using |
@@ -12,3 +12,4 @@ TODO | |||
/gh-pages | |||
/npm | |||
/dist | |||
tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this unrelated change?
I just put up #1367 which improves the test coverage of getIn and hasIn with tests that would fail for this PR which helps to illustrate my earlier feedback, perhaps you could rebase to include that? |
I borrowed your test and tested out a similar change in #1368 - that will both fix this new test while preserving a pass for the rest! |
resolves #1361
A pretty simple fix. However, it might not be 100% how the Immutable team wants to continue.
There's a default
NOT_SET
constant inTrieUtils.js
. HereSince
NOT_SET = {}
declaring thenotSetValue
fornull
andundefined
was left up to thewarn
call ingetIn
.However, based on the conversation in #1161, we can safely assume that
undefined
andnull
should be counted asNOT_SET
values as well.So, this PR resolves #1361 by including those values in the
NOT_SET
conditonal.Potential Improvements
Perhaps, a better (larger) solution would be to make
NOT_SET
a function that can compare against allNOT_SET
values (includingnull
andundefined
) so that this conditional can be brought back down to one statement, and this logic wouldn't have to be repeated elsewhere in the code base.