-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
Use throwIfNoEntry for default isFile and isDirectory #255
Comments
I'm not sure I understand what you're proposing. We use edit: maybe i misread. this is something node does support? which node versions support it? what happens differently with the option in a node version that doesn't support it? |
Sorry I should clarify: throwIfNoEntry is a node supported option that is much more performant than letting statSync throw and immediately catch it (https://nodejs.org/api/fs.html#fsstatsyncpath-options). For that reason, I believe we should use that option here. The option is a no-op in older versions of node, so long as you just keep the try/catch block the same. I will create a PR to showcase the change. My only concern with doing this change is that if a consumer is using a monkeypatched graceful-fs, they may run into issues with this change. Since this package does not specify graceful as a dependency, I dont think it is really a "breaking change", but its worth understanding that it may still break some people using a semi-common patch. |
It is none of our concern what someone does with graceful-fs; that's not a supported use case since it involves monkeypatching. A PR would be great. |
@ljharb I have linked the change here |
Similar to webpack/enhanced-resolve#316
Performance of skipping error generation is significant (mini-benchmark present in above issue) and the API is backward compatible.
Since this project doesnt use graceful-fs, this change should be available to make right now. I do somewhat worry about usage of monkeypatched graceful-fs, though, as that seems to fall into a weird grey area of deciding semver.
The text was updated successfully, but these errors were encountered: