New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle invalid clockspec error and log more when loading cache fails #9709
Conversation
if (e.message && e.message.includes('invalid clockspec')) { | ||
// Note this assumes only one snapshot file per cache and will potentially break | ||
// if multiple snapshot files exist in the cache directory | ||
const snapshotFile = readdirSync(options.cacheDir).find(file => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably want to log something before doing this because it can also fail
}, | ||
}; | ||
if (snapshotFile != null) { | ||
additionalMessage = readFileSync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to find the snapshot file? isn't it always written to the same location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's written to snapshot-<hash>.txt
but I don't know if the hash persists. I guess I can look through the code and see if the files location is available to be passed to loadRequestGraph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough ; okay, in that case you could look for snapshot-*.txt
rather than any txt file?
if (timeout != null) { | ||
clearTimeout(timeout); | ||
} | ||
let additionalMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can extract this to a function but maybe it can be simpler (avoid the guessing snapshot path part & just log more lines rather than assigning to a variable etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider extracting the new logging block into a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love your work 🍻
c8e633d
to
1daf2b0
Compare
↪️ Pull Request
💻 Examples
🚨 Test instructions
✔️ PR Todo