-
Notifications
You must be signed in to change notification settings - Fork 13k
Simplify statSync #59276
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
Simplify statSync #59276
Conversation
@typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
src/compiler/sys.ts
Outdated
// throwIfNoEntry is available in Node 14.17 and above, which matches our supported range. | ||
return _fs.statSync(path, { throwIfNoEntry: false }); |
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.
Potentially I can leave a try/catch here just in case someone uses something before 14.17; it wouldn't be faster but probably best overall since all supported users will stop needing to modify the stack lengh limit.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Not really helping, honestly. I don't know that this is worth it besides the cleanup... |
If we are keeping the |
You mean, you think this PR isn't worth it, or, that this PR is okay in the current state? |
Hm, maybe I'm not understanding the problem; this at least consolidates the try catching. If we're super confident that no polyfill will get this wrong, I can just revert the try catch. Mainly I was just trying to eliminate the writes to |
I've also read a bit and it seems like try/catch is free as long as any error is never thrown, which it won't be unless someone has a bad platform implementation. |
Not all of our stat calls went through the "faster" error stack path, so I fixed that. But, since we support only Node 14.17, we don't even need it since
throwIfNoEntry: false
is available.