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

Feat: Async implementation #46

Closed
spence-s opened this issue Mar 25, 2023 · 6 comments
Closed

Feat: Async implementation #46

spence-s opened this issue Mar 25, 2023 · 6 comments

Comments

@spence-s
Copy link

This lib is great, would there be a chance to support a fully async implementation even though ts doesn't directly support that AFAIK?

@privatenumber
Copy link
Owner

Yes, typescript methods are synchronous, so this was also designed to be synchronous to be a drop-in replacement.

Why do you need an async API?

@spence-s
Copy link
Author

spence-s commented Mar 26, 2023

It would be beneficial for performance in some applications and for code style preferences.

In particular, I'd like to use it alongside many other concurrent file io during linting.

@privatenumber
Copy link
Owner

Is there a specific problem you're facing? And how much of a performance gain is it?

@spence-s
Copy link
Author

spence-s commented Mar 26, 2023

No things work fine using it sync - and it's hard to quantify if or what the gain would be without having the async implementation to compare against.

It is quantifiable that if I use sync methods for other operations things slow down a lot. For me this would likely be just a small optimization for particular use cases where lots of files are being linted, but it would be nice to have all the file io be async for everything for my own stylistic preferences.

If you would be open to it, I would be willing to contribute this feature. Which would add an async version of each exported function. Along with analogous async tests. However, id also understand if you wanted to keep the scope of this project to be an exact drop in replacement.

@privatenumber
Copy link
Owner

I'm not convinced about the benefits of having an async version. In most cases, it can't be used due to limitations like supporting any CommonJS resolution that needs to be synchronous.

I understand that async versions are supposed to be non-blocking and theoretically faster, but I'm a bit skeptical about whether there's any noticeable performance improvement. I've come across reports suggesting that the async version of fs is actually slower than its sync counterparts (nodejs/node#41435, nodejs/node#38006, nodejs/node#25741). I'm not sure if I've ever seen anyone demonstrate notable performance gains.

At the moment, it seems like introducing async functionality would only add a significant maintenance burden without any clear justification.

If you have more data that can demonstrate a real performance gain, I'd be interested in taking a look. It would be helpful to see concrete evidence before considering the implementation.

@spence-s
Copy link
Author

@privatenumber no worries, those reasons make sense. Still, thanks for your work on this package, has been a big help.

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

No branches or pull requests

2 participants