-
Notifications
You must be signed in to change notification settings - Fork 75
Changing null -> undefined potentially dangerous #65
Comments
Do these problems exist only because we're codemodding away from something which uses a different convention, or is Looking at it another way, if we could rewrite the semantics of The change from EDIT: The obvious counterargument is that if we treat |
It's a fair question, and one I asked myself before posting, but yes, I do think these are actually intrinsically problematic, not simply because of existing precedent of
Could you clarify? If I have Re: rewriting
These are all cases where |
Thinking some more, I think it basically comes down to whether you expect |
Sure, reducing to the simple case of As I mentioned above, one feasible counterargument is that, since there is a semantic distinction between
|
@0x24a537r9 I think the question is, if you authored your code so it never distinguished between null and undefined (which is a common approach in the JS ecosystem), would this concern still apply? |
@0x24a537r9 But how you gonna distinguish between null is being value of b or a? I think this distinction is necessary. When I do sorry for the bad english btw. |
@fishythefish Thanks for clarifying. I see your point, and I think it's a reasonable interpretation of the operator. I think the thing is whether you expect the operator to be "about" the final value or "about" the navigable chain. I expect the latter given What I like about the @ljharb Yeah, the issue is that JS itself distinguishes between
It is also common and useful to distinguish between
And there are also differences when you interoperate with other languages that don't support
@ramesaliyev That is an issue, yes, but you have the same thing with |
None of the two interpretations is harder to polyfill than the other. You use the transformation const idx = (x, c) => x == null ? x : c(x); or: const idx = (x, c) => x == null ? undefined : c(x); |
@claudepache uhhh... lemme think... yes. Yes, you're correct, except in the case of |
It's the opposite; All of the cases you mention for differentiating are valid; but i wouldn't use optional chaining with those - and specifically, in all those cases, the explicit |
@ljharb whoops, yes. I need more sleep. Edited that comment to reduce confusion. The thing is that while you indeed may not, the fact that you're in a TC39 conversation means that you're probably a lot more knowledgeable and conscientious about JS than the average dev ;) The average dev probably will unwittingly pass in a What I'm proposing is a pragmatic approach to how devs will probably end up using the operator rather than how they theoretically should. I think I buy @fishythefish's argument that What do you mean by "the explicit |
// pretend this is inside React
var result = renderComponent();
if (typeof result !== 'undefined') { throw new Error(); }
var nodeType = result?.type; // either `undefined` if result is null, or `result.type` |
@0x24a537r9 Thanks for your detailed feedback here. It's incredibly valuable to see the result of the work you've put in to prototype The semantic alternative of returning Note that, if we go with the "null-preserving" semantics, then for cases which want |
"I think I buy @fishythefish's argument that undefined is somewhat more abstractly consistent for one interpretation of chained ?.s, but when a && a.b && a.b.c is so common as a pattern, I think following that pattern's behavior precedent is more ergonomic and safe (I'd venture to guess that's why idx does it that way.) typeof NaN === 'number' and 'string' instanceof String === false may be abstractly valid and make sense if you really think about them, but are clear footguns. I worry about the (naive appearance of) conversion of null to undefined here also being a footgun--something you always have to watch out for rather than something that encourages robust code." I don't know that it is correct to call this a "conversion of null to undefined". I think @fishythefish's explanation of the ?. operator is quite valid as he described it. If you have That, at least, is how I would read that statement. When I write |
This. |
@jrista Yeah, I think that's a very reasonable interpretation. And you're right it's not really "converting" anything--just destroying information. I meant it more in the sense of @fishythefish's original comment:
Out of curiosity, I posted this poll to r/javascript to see whether the average dev will expect |
The poll (and reaction emoji here) seem to pretty decisively favor I'd like to defend @ramesaliyev's point. @0x24a537r9, you write:
JavaScript currently loses information about whether or not variables are It's pretty easy to detect whether or not |
Let me put it this way: function f(arg) {
arg === 1 // f(1)
arg === null // f(null)
arg === undefined // ambiguous: f() or f(undefined)
}
a[b] === 1 // element b exists and is 1
a[b] === null // element b exists and is null
a[b] === undefined // ambiguous
a?.b === 1 // a exists and has property b, which is 1
a?.b === null // a exists and has property b, which is null
a?.b === undefined // ambiguous In all these situations, only |
|
I'm logically onboard with the op always returning |
@markkahn as has been explained upthread, it loses the information that the object is null. |
@ljharb -- I believe you misread my statement. I'm asking what the downside to |
I don't really understand the argument that a proposal should match behavior of existing idioms (e.g. To me, codemodding
The downside is that Consider: type Project = {
owner: ?User
}
type User = {
id: number
}
async function findProjectOwner(projectId) {
const project = await get(`/api/project/${projectId}`); // may return null
if (typeof project?.owner === 'object') {
return get('/api/user/${project.owner.id}');
}
} |
I think it needs to be asked, here, what is the operator really doing? IS the operator in the case of The propagation of information would need to be an explicit trait of the operator. I mean, what if Again, I would be looking for the value of Is the desire to propagate Should we resort to |
This issue is asking it to do |
@ljharb: This is why I asked, to see what people think is the case. ;) That is not what it is doing. According to the spec in the main readme: https://github.com/tc39/proposal-optional-chaining#base-case
Someone posted a poll earlier in the thread, and it appears the above was overwhelmingly chosen by the community. That indicates this is how most javascript developers expect the operator to work. This is the logical and semantically correct outcome as well. |
ha, i see what you mean. my gut reaction is as above; but now thinking more about it, by returning |
@jrista The closest analog to my proposal was more @ALL: With now twice the data, I think the answer about what is "more expected" is pretty clear: I think this poll (as unscientific as it was) clearly demonstrates at least two facts:
Thanks all for an enlightening conversation! Feel free to close unless there are any further thoughts! |
@lhorie -
My argument to this is the same one you made -- "I don't really understand the argument that a proposal should match behavior of existing idioms" That line can easily be replaced with:
In other words yes, it's different behavior but I don't see any ways in which it's worse |
@markkahn To me, it seems extremely bad that |
@lhorie -- Sorry, I misunderstood your original intent. Still, |
It's not equivalent semantically exactly for the reason you described. You're replacing a whitelist check (i.e. only objects) with a blacklist (not nulls and not undefineds). Generically speaking, you're suggesting replacing a more strict check with a less strict one. Doing that would be similar to blindly codemodding
That can be said about many of the regressions the OP experienced. Regressions due to reliance on I see it as a feature that |
I'd like to point out that C# will return |
Although it is still unclear for me that the concrete hazards raised in the present thread would not just be inevitable refactoring hazards, I do think that having |
I like the behaviour of returning undefined always. This makes comparisons easier for example |
Couldn't you just do:
With a nullish check? That should cover null and undefined, but not other falsy values, right? |
@jrista not if you want to avoid all the pitfalls of javascript type coercion https://dorey.github.io/JavaScript-Equality-Table/ |
for example if |
@cloudkite if it returns null or undefined, |
@ljharb personally I write my code to not distinguish between null and undefined. So I don't have a usecase for treating them differently. Maybe if you really want to distinguish between null and undefined you need to fallback to ternaries? I think we should optimise for the most common case. I hope the most common case is not distinguishing between null and undefined 🙏. But if the opposite is true then I'll make my peace with it 😊 |
I’m asking because i think it’s important to weigh the relative difficulties of both, in each of the scenarios. |
Here's my argument for The value ( One of the use cases I see for the nullish coalescing operator is assigning multiple properties onto an instance. It gets quite messy to see a dozen lines of Let's say the option is a foreign key. If the optional chaining operator decides on To me, this is a perfectly valid use case (assigning properties on an instance in a REST API). I'm currently working on something nearly identical to this, and found it to be a great example of where the value in question would matter. In this instance specifically, it's inevitable that the decision was made to treat Thoughts? |
I agree that there are useful and nuanced differences between undefined and null. In many cases they can be treated the same, however in some cases, they are not treated the same by runtime or platform. One of my favorite use cases is when I need to control what data is serialized out of a REST service. Undefined will not serialize, whereas null will. An often useful distinction. If using express and node, for example, which runs on V8. With V8, any time you This is then the most efficient way to prune a result from a REST service. Set properties I do not wish to serialize to Undefined and null are not the same thing. They are different, and sometimes those differences matter. Even if in a majority of cases they do not, we shouldn't lose sight of the fact that they are indeed not the same thing. |
One of the key comments on why we should be using So take for example the
Here we lose information about the fact that Now take the
We still lose information about The key question of this thread seems to be whether the operator is trying to find information about There is actually no situation that we return the value of |
@nick-michael You can a = null;
(a ?? undefined)?.b // undefined
a = { b: null };
(a ?? undefined)?.b // null You can't easily do that with the "always undefined" style. |
@nick-michael imagine |
I don't know... I have seen all sorts of patterns used for this purpose. |
I guess it depends what you would classify as "easily" Personally I feel like: BUT I would have to admit that when you start going another level deep, it starts to get a bit more horrible when using the lambda expression, although both get pretty bad: a === null ?
a
: a?.b === null ?
a?.b
: a?.b?.c
((a ?? undefined)?.b ?? undefined)?.c Edit: Can be simplified further as pointed out |
Arguments can really be made both ways (as we've seen here!), but from my point of you the strongest case is the one I mentioned previously in that the optional chaining operator consistently returns the value of I also saw your comment in #69 with the table of current utility libraries which I think is a pretty strong case for what most JS developers would expect the outcome to be |
((a ?? undefined)?.b ?? undefined)?.c can be simplified to (a.?b ?? undefined)?.c That said, I still prefer the always-undefined semantics to a small degree, just because it feels more intuitive to me and the way I see this operator. Personally, I don't think I have ever needed to disambiguate between a null intermediate value and an undefined "final" value. And if I ever need to, all the more verbose (and flexible) tools that we use today would still be there to help. The "feels more intuitive" part is highly subjective, of course. I'd be happy with either one. |
overhauled proposal
Now that the proposal has reached stage 4, the semantics of (null)?.b is fixed. Indeed, conversion between |
At Facebook, we recently adopted a subset of
?.
in our codebase, but the codemod fromidx
(see https://github.com/facebookincubator/idx) to?.
has introduced a number of bugs related to?.
always returningundefined
, even when the original value wasnull
. I see in the proposal:In practice, this has quickly been found pretty unsafe, with the move away from
idx
's null-preserving style bringing a number of pretty significant and non-obvious bugs. I'd like to propose switching?.
to follow a moreidx
/||
/??
-like model of returning the first nullish value (null
orundefined
) rather than alwaysundefined
to avoid 4 main classes of bugs that result from this (likely unintentional) value conversion:undefined
values as props ("Expected propx
to be supplied toRelay(MyComponent)
, but gotundefined
. React throws an invariant exception if you returnundefined
from a render function (which led to an app crashing bug at Oculus).defaultProps
and function parameter defaults (undefined
will use the defaults, whereasnull
will be passed through asnull
)3.a. when they're intentionally treated differently (because
undefined
represents the nonexistence of a value butnull
represents a value of nonexistence)3.b. when they're unintentionally treated differently (this caused another major bug internally when
!== null
was used instead of!= null
)Number(null) === 0
butNumber(undefined) === NaN
In all cases, the unintuitiveness of
?.
convertingnull
s toundefined
s can cause (and has already caused) major bugs in otherwise sensible-looking code. Are there reasons not to go with a semantic-preserving system likeidx
and||
, which seem to be a bit safer?The text was updated successfully, but these errors were encountered: