Skip to content
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

Don't use exceptions to handle optimistic file opens in the compiler. #39740

Closed
5 tasks done
MicahZoltu opened this issue Jul 25, 2020 · 4 comments
Closed
5 tasks done
Labels
External Relates to another program, environment, or user action which we cannot control.

Comments

@MicahZoltu
Copy link
Contributor

Search Terms

compiler exception fileExists

Suggestion

Re-implement the fileSystemEntryExists function in the compiler such that it doesn't use exception handling to determine folder presence.

Use Cases

When using ts-node, if you turn on "break on all exceptions" when trying to track down a bug in some TypeScript code, hundreds of exceptions will be thrown from the TypeScript compiler as it tries to find files on disk by probing a number of locations for each file. At the moment, this makes the "break on all exceptions" feature of my debugger essentially useless unless I can get a breakpoint set before the error in question occurs but after TS has loaded everything which isn't always possible (especially when using dynamic imports).

Examples

N/A I think, let me know what I should put here if something is desired.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@RyanCavanaugh RyanCavanaugh added the External Relates to another program, environment, or user action which we cannot control. label Jul 25, 2020
@RyanCavanaugh
Copy link
Member

This is still the fastest method that we have for checking file existence and we don't intend to slow down everyday builds for the sake of making debugging easier; we have a PR up at nodejs/node#33716 which you can voice support of

@MicahZoltu
Copy link
Contributor Author

This makes me sad, but I understand and accept your reasoning. Closing until such time as nodejs/node#33716 is fixed.

@weswigham
Copy link
Member

It's also the recommended pattern for (sync) fs operations in node: Checking for existence, then performing the operation introduces a race condition (the file may be deleted from the disk between the two calls; the filesystem is an inherently racey place). You have to handle the exceptional case anyway (so to avoid the extra hit on disk, you may as well not bother checking first).

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Jul 26, 2020

An uncommon race that results in an exception is far less impactful than having the exception thrown every time.

try {
    if (fileExists(path)) {
        const file = readFile(path)
        ...
    } else {
        // hot path for file not found
        handleError(new Error(...))
    }
} catch (error) {
    // rare race condition where the file is deleted between the check and the read
    handleError(error)
}

For the double disk hit, something like tryReadFile would be nice to have:

// hot path 
const file = tryReadFile(path)
if (file === null) {
    // TODO: generically handle read failure (e.g., move on to look elsewhere for the file), you can always go back and use the throwing versions if you want more specific errors after you have probed multiple locations on disk and they all failed
}

To be clear, I don't think that these patterns are necessarily great patterns for the novice developer who just wants to read a file off disk in their script. However, the current NodeJS API doesn't expose the necessary primitives to do these more advanced patterns, such as probing the filesystem for a file.


EDIT: Bleh, just realized this is the TS issue and not the NodeJS issue! Most of the above was meant for NodeJS developers, since I know that this is all out of your control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Relates to another program, environment, or user action which we cannot control.
Projects
None yet
Development

No branches or pull requests

3 participants