[feature] export types

[KostiantynPopovych]

This PR fixes WebStorm's eslint resolution error, adds types into the dist folder for further import & usage of depngn as a function, and fixes eslint errors throughout the project.

[kindoflew]

i think we should probably keep this try/catch because the Promise.all in getYarnEngines could throw, which I believe would cause an Unhandled Promise Rejection without it.

there's a try/catch in readJsonFile -- if it throws an unexpected error, it would cause Promise.all to reject and it isn't being caught at the call site. I don't think rejections like this (Promises across functions) bubble up to the next highest catch block, but I'm not positive.

or, alternatively, we could put a try/catch around the code in getYarnEngines.

[kindoflew]

after reading more, i think the right move is putting the try/catch around the code in getYarnEngines

[kindoflew]

same thing here, i think -- callback could technically be any async function -- if it's a Promise that can reject, this would lead to an Unhandled Promise Rejection

[KostiantynPopovych]

I got your points to keep the catch block, but I'm not sure that I get the previous logic of re-throwing errors:

} catch (error) {
  throw error;
}

So, should we do something specific in these catch blocks then?

[kindoflew]

@KostiantynPopovych in some of them we should (and we do, when we know certain errors won't lead to unexpected behavior. like if readJsonFile doesn't find the file it's looking for, we don't exit the app -- we just say that value is undefined in compatData).

but for others, i think it makes sense to throw the error so it bubbles up to the function it's being called in. not to mention, i tend to take an "err on the side of caution" approach when it comes to error handling.

i also don't think it hurts to have try/catch blocks that might not be 100% necessary -- if anything, it at least makes it explicit it that the code it wraps can end up in an error state. and it makes adding specific error handling a little easier in the future (like if we discover a some new, specific type of error isn't app-breaking, we can just add a if (error.message === 'whatever') check and handle it in a catch block that's already there).

i guess that's my plan for these that just re-throw -- we know, at the very least, the error will reach the top-level catch where we log it and set process.exitCode = 1. if someone opens a bug with an error message and it turns out the error shouldn't stop execution of the rest of the app, we can just add specific handling for that case.

however, i do think my try/catch blocks are so liberally placed because i don't fully understand all the different behaviors of try/catch (and throwing in general) in JS. might be a good topic for my first guest blog post 🤔 (unless you want that topic for one of yours -- feel free to take it lol)

[arielj]

I agree that catch and only re-throw the error is not that useful, errors already go up the call stack anyway so this is adding extra try/catch overhead, I would vote to remove those too

I do see the reason behind being explicit like "I know this can throw an exception" because this is a big issue with JavaScript in general and there's no simple way to fix it, but I don't know if adding a try/catch is the best solution (I think it's something the language should fix, or maybe a function name convention like in rails a method with ! in some cases means this can raise an exception for example)

[kindoflew]

well for execWithLog, i think just re-throwing the error is the best course of action -- we have no idea what the error could be because the async function could be anything (even though, right now, we're only using it in one place and we mostly know what errors it can throw 😅).

in getYarnEngines, we'd only re-throw so we don't get an Unhandled Promise Rejection -- and this is only for the case where the error is unexpected. we're already handling the case where the JSON file doesn't exist -- we just set empty strings for that package's engines data.

in readJsonFile itself, we also have to re-throw to avoid Unhandled Promise Rejection.

besides those cases -- i think re-throwing is valuable for debugging purposes. when we re-throw, the stack trace gets thrown along with it. so when it's finally logged, the trace starts at the actual function that threw originally, instead of the function that the error bubbled up to (even if the error originated in one of our dependencies).

as far as overhead is concerned -- i think that might be an (unnecessary?) optimization at the expense of easier debugging. and, since we're no longer fetching from npm, performance isn't as much of a concern anymore (at least in terms of execution time).

plus, it looks like we're only re-throwing in 5 places -- the 3 I mentioned above, getPackageData (which has conditional, non-re-throw error handling as well), and getDependencies (which might not actually need it, but it looks like it'd be preserving the stack trace if readJsonFile unexpectedly throws).

[arielj]

but if you re-throw an error you still have to handle it catching it somewhere else, right? I'm missing why the re-throw is needed since exceptions already bubble up the stack and you can always catch them when you want to deal with them and you can debug things the same way

for execWithLog you say it's better to catch and re-throw because you don't know what errors you can have, but it's the same, you'll be just re-throwing whatever error you get so you still have to do another try-catch somewhere else that will have to catch whatever error execWithLog threw, so you end up with 2 try/catches for one exception

maybe I don't see the exact use case where a re-throw is better for debugging than just letting the exception go up normally, one could argue that having the complete stack trace from the original call is better than having the stack trace starting where you re-threw it, at least for me more information of an error is better (or I'm seeing this backwards?)

EDIT: I don't want this to take forever to be discussed though, I just think that over-pessimistic (worrying about unexpected errors) code is a bad pattern, everything can fail at some point so we would need try/catch just in cases in a lot of places

[kindoflew]

they don't always bubble up the stack -- Unhandled Promise Rejections don't across function boundaries (this is my main reason for re-throwing in execWithLog -- if it's a Promise that can reject it has to be re-thrown to bubble up or it will be "unhandled").

re-throwing does preserve the original call site. it's if we did something like throw new Error(error) that we'd lose the original call site.

also, i read some more and i think i misunderstood how stack traces are built in Node. so most of my original argument (besides Unhandled Promise Rejections) was wrong.

and also also -- yeah, this conversation doesn't have to drag out. I just miss learning and BSing with you guys and it's so much faster in Slack lol.

So to speed this up, what if:

• we try/catch when it can be a Promise that can reject
• we try/catch when we want to handle a specific error in a specific way
• we let it bubble up otherwise (adding new try/catch blocks when/if the need arises -- through opened bug Issues or whatever)

[KostiantynPopovych]

Hey @arielj @kindoflew, I reverted the logic of catching promises, but I also changed the build method in favor of using tsup. This migration reduced the amount of build-related dev dependencies to two - tsup and @swc/core (this is needed because we specified "target": "es5". Sorry for adding it to this PR; I can try to omit it if necessary.

Also, I just tried to run builded version of the depngn using node v8 and it also works, so should we change the engines field to be >= v8.17.0?

[kindoflew]

works for me! i'm not married to rollup or anything -- i just used it because I knew it relatively well and it's easier to set up than webpack haha

as far as changing our engines field -- if we confirm it works for everyone on that version, it's probably the right call. maybe we confirm it also runs on Linux and Windows on v8? i don't know why it wouldn't, but just in case?