You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hi @ilyapx, thanks for the PR, I'll mark it to be merged in next major release, in case, somehow someone is expecting to get the result in the next tick.
I think that this has a very negative impact on users of this library. When passing a callback to a function there is an implicit contract that the function falls into one of two categories. 1) function that uses the callback n number of times, here the callback is generally not even called callback, e.g. forEach, map. 2) functions that performs io or computations and then later returns an answer.
When using functions that are in category 2 you expect the callback to be called later, since you expect for some work to be done while the event-loop keeps ticking on. Not making this very clear to the user generally leads to releasing zalgo (blog post by isaacs). That is very bad.
I very much feel that the best thing to do would be to remove the callback entirely since it serves absolutely no purpose. There is no work being computed in another thread. There is no asynchronous IO taking place. In fact, everything will be computed before the function have returned, wether you are passing a callback or not.
The very fact that the callback parameter exists is something that has been confusing me when using this library a number of times. Since nothing asynchronous is happening it makes about as much sense as adding a callback to a function that concatenates two strings together.
[...] because it rather hard to make changes to the API.
It was released as a breaking change though...
Also, have there been any measuring of the performance? I cannot imagine this being the bottleneck in any normal app at all? Especially under load this will defer back to the event loop so it could potentially make the throughput worse under heavy load due to starving of the event loop. Anyhow, I don't think it will make any difference at all...
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
running synchronous code asynchronously using process.nextTick
has a negative latency impact