scopsy / await-to-js

Async await wrapper for easy error handling without try-catch
http://blog.grossman.io/how-to-write-async-await-without-try-catch-blocks-in-javascript/
MIT License
3.27k stars 153 forks source link

Every CATCH has an IF #33

Closed AckerApple closed 3 years ago

AckerApple commented 5 years ago

I almost love this library. I hate the second argument.

Every catch will result in running an if condition. Just too costly for my blood.

I looked at await-of and it has no IF but it returns with the value first and the error second. Hate that more but it has no inner IF logic.

I looked at await-on and thats just a no no no no.

Would you accept a pull request if I added another function that does all the same EXCEPT for no second argument and no catch IF condition?

ComBarnea commented 5 years ago

@AckerApple in that case you will end up loosing the error, right? What will you do if something did happen and there is no answer?

AckerApple commented 5 years ago

I don't think you are understanding me correctly.

Why do you have this extra argument? (rhetorical question) https://github.com/scopsy/await-to-js/blob/master/src/await-to-js.ts#L8

Why support such a feature in this unit test? (rhetorical question) https://github.com/scopsy/await-to-js/blob/master/test/await-to-js.test.ts#L24

The extra argument "errorExt" is a distasteful hit against performance. Every catch, will result in an IF condition being processed even though never needed.

I work for FirstData (now Fiserve) and we processes hundreds of thousands of records every couple of hours for payment processing. We have a lot of async/awaits that resolve in catches as we have our validations throw errors. I can't adopt this package into our company without addressing this IF that runs with every catch.

I would like to remove the extra argument OR add an additional function that has no second argument.

Understand? I'm in Florida preparing for a storm. At some point I'll just issue the pull request.

AckerApple commented 5 years ago

Pull request #34 submitted.

If this was my package, I would just remove the second argument and its accompanying logic. The extra argument isn't even documented so no one but you is most likely even using it.

Please thoroughly consider my request. Your await-to-js is the best approach of the 3 packages on npm and I don't want to release a 4th just over this errorExt argument.

Thank you kindly

AckerApple commented 5 years ago

@ComBarnea

I suppose you are trying to ask me is: What if a promise is rejected with like Promise.reject(null), right? Meaning, what if a rejection happens and NOTHING is returned. Is that what you are asking?

Who does that? The concept of failure without a reason, that's terrible. But I suppose you are saying it can happen.

IF that's what you're asking me, then your code is NOT going to handle that type of situation.

The code on this line uses an Object.assign(). https://github.com/scopsy/await-to-js/blob/master/src/await-to-js.ts#L14

IF you try to use Object.assign(null, {something:22}) on a null or undefined object an error of "Uncaught TypeError: Cannot convert undefined or null to object" occurs.

adi518 commented 4 years ago

@AckerApple At this point, #34 will probably never get merged. See this. No If condition there. Although I like the array destructure pattern more, that one returns an object. There might be room for one more package, one that returns an array without the extra jab.

AckerApple commented 4 years ago

@adi518 thank you so much. That package looks almost great other than it has a dependency on @babel/polyfill for seemingly no reason.

As seen here: https://github.com/krlozadan/a-promise-wrapper/blob/master/src/index.js

adi518 commented 4 years ago

Ah yes, I did notice that too. He made a mistake adding it. Stuff like that should be added by consumers. It's worth starting an issue, or, unless npm has absolutely no more options, I'll just make a new one that combines the good and leaves out the bad.

JohnForster commented 4 years ago

@AckerApple The to library can be written in one line of code.

const to = promise => promise.then(x => [null, x]).catch(e => [e])

You don't need a library for this, a util file will do.

AckerApple commented 4 years ago

Thank you and yes that would do. It’s not data typed so often that one liner is gonna lead to “any” translations but yeah it works.

JohnForster commented 4 years ago

@AckerApple

const to = <T>(promise: Promise<T>): Promise<[null | Error, T | undefined]> =>
  promise
    .then((res) => [null, res] as [null, T])
    .catch((e) => [e, undefined] as [Error, undefined]);

if you need type safety

AckerApple commented 4 years ago

Well done. Gonna move that code into my own package and install everywhere I was using this package. If it was a one liner than yeah maybe I copy paste for every use.

We stopped using this code package a long time ago and ONLY because it had an unnecessary IF that is a major performance hit when batch processing millions of financial records (FirstData)