Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Convert fs.*Sync to async equivalents internally #3333

Closed
JoshuaKGoldberg opened this issue Oct 17, 2017 · 3 comments
Closed

Convert fs.*Sync to async equivalents internally #3333

JoshuaKGoldberg opened this issue Oct 17, 2017 · 3 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

Spinning off of #2328: it's generally bad practice to use synchronous fs APIs, as they are blocking. Using async equivalents will make it easier to integrate with asynchronous code and I'd love to see some internals' file operations parallelized if possible for performance.

@ajafff
Copy link
Contributor

ajafff commented Oct 17, 2017

Switching to async IO in runner.ts and rule tests is definitely something we should do.
While rewriting the code we should try to parallelize IO. Your PRs tend to await the promise immediately, thus making everything run sequential again. That doesn't give any benefits as it only adds more overhead.

Regarding changes to Linter:
I don't see any benefit here. Linting is a CPU bound process. There's no IO except for fixes (and maybe some IO in Rules, but currently they cannot be async anyway). I guess users expect a (slightly) increased runtime while autofixing.
My main concern is about compatibility with API users. Some of them need a synchronous API.
I just looked at tslint-language-service. It seems the TypeScript language server requires plugins to implement sync APIs. /cc @egamma @angelozerr

We can probably reconsider this as soon as TypeScript starts to support async APIs

@JoshuaKGoldberg
Copy link
Contributor Author

Your PRs tend to await the promise immediately

Agreed, this is sub-optimal. My hope was to trivially change the APIs to async/await to get the API modified, then rewrite.

compatibility

I'm up for waiting - will try to remember to come back to this if and when the APIs allow async operations.

@egamma
Copy link

egamma commented Oct 23, 2017

@ajafff @JoshuaKGoldberg

My main concern is about compatibility with API users. Some of them need a synchronous API.
I just looked at tslint-language-service . It seems the TypeScript language server requires plugins to implement sync APIs. /cc @egamma @angelozerr

@ajafff is correct, the tslint-language-service runs inside the TypeScript server and the TypeScript host APIs are sync APIs. For example, computing the lint warnings is expected to be a sync operation, see https://github.com/angelozerr/tslint-language-service/blob/88791766ec122d0cf315af7502d38049549b9837/src/index.ts#L304. An async lint call would be breaking and we would also need a sync operation.

The situation is different in the vscode-tslint extension, there async APIs are supported as well and an async lint could be consumed, but the benefits are not clear to me yet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants