fix optional chaining undefined for non macros #2215
Conversation
patricklx
force-pushed
the
optional-chaining-undefined-for-non-macros
branch
from
c5dff80 to
b58baec
Compare
| aKnownValue.foo = true; | ||
| result = aKnownValue?.foo; | ||
| `); | ||
| expect(code).toMatch(`result = aKnownValue`); |
There was a problem hiding this comment.
There are 2 runs with the same test. with and without preset env. So one converts it to manual null check, cannot make one version that matches both results
There was a problem hiding this comment.
Interesting one!
It seems the root cause of this is our Evaluator returning a confident result for that expression, which itself delegates this to babel's own NodePath#evaluate() which again returns a confident result. Seems basically the same as what @ef4 reported here already: babel/babel#14197
Even when that bug got fixed, I think the change here is good, as we only want to take the cost of that evaluation in the context of some get*Config call, which is basically what this PR is doing, right? So I wouldn't be surprised if this has also some performance benefits!
|
thats, right. only if its inside, or rather "part of" a get*Config call |
|
I just patched the dependency with this PR and it does indeed fix our issues :) Thanks a lot for looking into it so quickly! |
|
Anything that still needs to be done for this? Looks like one check is failing due to a browser timeout but seems unlikely that this change caused that failure 🤔 |
|
If we're able to produce incorrect code in the case of non-macros, we are able to produce incorrect code in the case of macros too. So we need a real bug fix that eliminates the production of invalid code. This only limits the scope of the bug, without fixing it. |
|
So, need to wait on babel/babel#14197. |
|
In the process of explaining how to do this it turned I just did it: #2286 |
fixes #2212
fixes #1812