Fix up code so we don't crash when TS itself is emitted with let/const #50151
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| case AssignmentDeclarationKind.ModuleExports: | ||
| valueDeclaration ||= binaryExpression.symbol?.valueDeclaration; | ||
| const valueDeclaration = kind !== AssignmentDeclarationKind.ModuleExports |
There was a problem hiding this comment.
Was this intended to be equivalent? It doesn't look equivalent (and it isn't obviously related to let/const).
There was a problem hiding this comment.
+1 : This is not equivalent but clearly tests are not failing..
There was a problem hiding this comment.
It looks equivalent to me... Am I missing something?
There was a problem hiding this comment.
If it's any of the original three, it'll not be equal to the 4th one, so it picks the first branch, otherwise it's ModuleExports, so it picks the second branch. I tried to keep them in order, but maybe it'd be clearer to just not use an if, and copy the two lines to both blocks.
There was a problem hiding this comment.
Oh, I see. If it was one of the first few cases but the expression turned out to be undefined, it'll pick the second expression even if it's not ModuleExports. That is different!
There was a problem hiding this comment.
I checked, and in all of our test cases, if binaryExpression.left.symbol?.valueDeclaration is undefined, binaryExpression.symbol?.valueDeclaration is also undefined, hence why my PR doesn't fail even though the logic is different.
There was a problem hiding this comment.
I’m sure I wouldn’t have written this as a falls-through and ||= if I could have combined the cases like you have here, but it’s disappointing the tests don’t capture that. It’s possible I was being overly defensive.
There was a problem hiding this comment.
I'm OOF but had some time in the airport, changed it so it's semantically equivalent to be safe.
There was a problem hiding this comment.
I'm a little worried about someone adding a case and missing the exception, but I agree that it looks equivalent now.
There was a problem hiding this comment.
The alternative is to just use var, if you all find that preferable.
| @@ -1363,6 +1363,11 @@ namespace ts { | |||
| onUnRecoverableConfigFileDiagnostic: noop, | |||
| }; | |||
|
|
|||
| // The call to isProgramUptoDate below may refer back to documentRegistryBucketKey; | |||
There was a problem hiding this comment.
That's scary, but I don't see a better way around it. @sheetalkamat might.
Either way, thanks for the comment!
There was a problem hiding this comment.
Seems like its needed only for config file's json and we currently dont use that since we implement getParsedCommandLine on project.. But yeah this change looks good
|
I know this is approved (so people must be happy enough with it), but would it be preferred for this to be Happy to do it either way. |
Please don't use |
Alternative to #50141 split out from #50022 where I was performance testing the cost of let/const.