fix: eliminate "Cannot read properties of undefined (reading type)" error #5443
Conversation
| @@ -280,7 +280,7 @@ export abstract class RetrieveExecutor<T> extends DeployRetrieveExecutor<T> { | |||
| protected async postOperation( | |||
| result: RetrieveResult | undefined | |||
| ): Promise<void> { | |||
| if (result) { | |||
| if (result && result.response.fileProperties !== undefined) { | |||
There was a problem hiding this comment.
Can be done with
| if (result && result.response.fileProperties !== undefined) { | |
| if (result?.response?.fileProperties !== undefined) { |
There was a problem hiding this comment.
I dont understand this code well enough, but the question that I have is why is the check
result.response.fileProperties !== undefined not at line 288 juts before calling setPropertiesForFilesRetrieve ?
There was a problem hiding this comment.
I put my condition with the other condition because that was where it originally was. It's pretty old code so I just kept the status quo. 🙂
I would assume it's because putting the if statement earlier means Lines 284-288 wouldn't be run unnecessarily.
Co-authored-by: peternhale <peternhale@users.noreply.github.com>
| @@ -280,7 +280,7 @@ export abstract class RetrieveExecutor<T> extends DeployRetrieveExecutor<T> { | |||
| protected async postOperation( | |||
| result: RetrieveResult | undefined | |||
| ): Promise<void> { | |||
| if (result) { | |||
| if (result?.response?.fileProperties !== undefined) { | |||
There was a problem hiding this comment.
not sure if my comment got lost, so reposting.
not knowing the code well enough, a naive question. why is check fileProperties !== undefined not before calling setPropertiesForFilesRetrieve?
Also can setPropertiesForFilesRetrieve be called in other places? which means does this check need to go in setPropertiesForFilesRetrieve ?
There was a problem hiding this comment.
Also as per this fix, everything after line 283 will be skipped even if result is not null . is this expected?
There was a problem hiding this comment.
-
Copying over above answer - I put my condition with the other condition because that was where it originally was. It's pretty old code so I just kept the status quo. I would assume it's because putting the if statement earlier means Lines 284-288 wouldn't be run unnecessarily.
-
As of now,
setPropertiesForFilesRetrieve()is not called anywhere else, according to a search.
There was a problem hiding this comment.
@diyer That is a good point. The check if (result?.response?.fileProperties !== undefined) { guarantees the block will not be executed if any property named is undefined. The previous block ran when result was not undefined, it might be useful to better understand which parts of result are used within the block, including the createOutput call, so see if additional not undefined checks need to be done for each operation within the block. Is createOutput independent of PersistentStorageService.getInstance().setPropertiesForFilesRetrieve WRT individual properties of result.
There was a problem hiding this comment.
There's a slight difference in the contents of the Output tab. If the check is moved down to Line 289, there will be an extra blank line in the Output tab before the ended statement because output is an empty line and channelService appends it.
That's the only difference that I see - result is not used anywhere else in that block. But I guess it would be nice to clear the error collection in case there's something there?
daphne-sfdc
force-pushed
the
daphne/fix-retrieve-cancel-error
branch
from
5fbe3f4
to
f197108
Compare
What does this PR do?
Eliminates the "Cannot read properties of undefined (reading type)" error that happens when a user cancels a retrieve.
What issues does this PR fix or reference?
@W-15091722@
Functionality Before
When a user clicks the Cancel button while a retrieve is in progress, they see the "Cannot read properties of undefined (reading type)" error.
Functionality After
When a retrieve is cancelled, there is no error message and the command ends gracefully.
