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
reduce sync io, only ignore foreseen errors #234
base: master
Are you sure you want to change the base?
Conversation
@@ -88,12 +88,14 @@ retrieveFileHandlers.push(function(path) { | |||
if (xhr.readyState === 4 && xhr.status === 200) { | |||
contents = xhr.responseText; | |||
} | |||
} else if (fs.existsSync(path)) { | |||
} else { |
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.
both usages of fs.readFileSync
are already inside a try/catch
block, it's safe to just let them try the read - a missing file error (ENOENT
) gets swallowed anyway
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.
lgtm!
Hmm, I wonder if this should be a breaking change or not 🤔 What do you think? It might throw now when it didn't before... |
@LinusU I can understand if that's what is decided. Though, personally i view this as a fix. |
I suppose on principle alone its always better to play this safe... Basically this could "break" downstreams since all errors were being swallowed, before. Though, i would argue this would be a legitimate break - but probably better to release as major bump |
@evanw thoughts? |
fs.existsSync
preceding afs.readFileSync
is 2 sync io operations that can/should be 1.ENEONT
) errors should be swallowedEACCESS
is swallowed.