25667 - getModifiedTime has wrong return type#25680
Conversation
a-tarasyuk
force-pushed
the
bug/25667-getmodifiedtime-has-wrong-return-type
branch
from
177efea to
550710d
Compare
|
@sheetalkamat and @Andy-MS can you please review this change. |
|
|
||
| const inputTime = host.getModifiedTime(inputFile); | ||
| if (inputTime > newestInputFileTime) { | ||
| if (inputTime !== undefined && inputTime > newestInputFileTime) { |
There was a problem hiding this comment.
Can you do a single check up-front? Plus, it doesn't seem like we're now actually handling undefined, we're just doing observably nothing.
There was a problem hiding this comment.
Ok, will move. Maybe would be better to set a default value in order to avoid checking on undefined, like it was done in this case https://github.com/a-tarasyuk/TypeScript/blob/550710d2a2ce019dfc7ba8120b112e635fb5d9dc/src/compiler/sys.ts#L306. what do you think?
There was a problem hiding this comment.
If we haven't run into undefined errors before, we're likely always be passing valid paths. So dropping every exception at the implementation of function getModifiedTime using catch (e) { return undefined; } was probably the wrong choice in the first place.
There was a problem hiding this comment.
This condition has been changed because the program uses https://github.com/a-tarasyuk/TypeScript/blob/550710d2a2ce019dfc7ba8120b112e635fb5d9dc/src/compiler/program.ts#L194 sys.getModifiedTime where has been added undefined
There was a problem hiding this comment.
@Andy-MS we shouldnt assume that getModifiedTime would be called only on present files.
@a-tarasyuk You are on right track. Please use inputTime = host.getModifiedTime(inputFile) || missingFileModifiedTime; instead of additional checks.
There was a problem hiding this comment.
@sheetalkamat thank you very much. I'll add changes and re-open PR.
… bug/25667-getmodifiedtime-has-wrong-return-type
a-tarasyuk
force-pushed
the
bug/25667-getmodifiedtime-has-wrong-return-type
branch
from
428d710 to
8ae163a
Compare
| for (const file of outputs) { | ||
| if (isDeclarationFile(file)) { | ||
| priorNewestUpdateTime = newer(priorNewestUpdateTime, compilerHost.getModifiedTime!(file)); | ||
| const fileModifiedTime = compilerHost.getModifiedTime!(file); |
There was a problem hiding this comment.
Why cant you have this or with missingFileModifiedTime ?
|
@sheetalkamat thanks, fixed. |
4 participants
Fixes #25667