margau / dmxnet

ArtNet-DMX-sender and receiver for nodejs
MIT License
69 stars 21 forks source link

redesigned error handling #54

Closed Bannsaenger closed 1 year ago

Bannsaenger commented 1 year ago

I added the ability to use a own error handler in the parent application.

The background is, that i use the library together with Node-RED. If you throw the error in the library, the application exits because you cannot catch these error in Node-RED.

kev-ac commented 1 year ago

Great idea to refactor the error handling. I found this because I ran into a similar situation.

One note: The Typescript definition for DmxnetOptions has to be modified as well, otherwise the compiler throws an error: https://github.com/Bannsaenger/dmxnet/blob/a310f530a94e39f8cd1f61b55d4fc58dc288a1c8/lib.d.ts#L116

See my open PR.

margau commented 1 year ago

Hi, sorry for the delay. To be honest, I'm currently not actively working with dmxnet, due to time constraints and other projects. In general, this looks good - thanks! Can you remove the package.json changes from the PR, or are they strictly needed for this feature?

I think we can merge it then.

kev-ac commented 1 year ago

Yes I think the changes to the package*-files are not needed for this PR as none of the changes are dependent on any library. It surely was just an oversight to add the files to the commit.

I would suggest @Bannsaenger to merge in my changes to the Typescript definition while altering this PR to not run into issues in TS projects.

Bannsaenger commented 1 year ago

I removed the package* files from the PR. The changes from @kev-ac are merged and i added the esat code in the type script definition as well. Hope this is all right now to merge my PR

kev-ac commented 1 year ago

@Bannsaenger There might be an issue here. You've deleted the package.json and package-lock.json in two commits, so they would be deleted from the repo if this gets merged. Might be worth to revert and maybe squash your commits and have a look at 0231f9b where these files were altered.

Bannsaenger commented 1 year ago

Ok, i synced my files with these from margau repository.

kev-ac commented 1 year ago

I've created a separate PR in #55 to remove the confusing commits of removal and adding of package files. Please feel free to review it.

margau commented 1 year ago

Closed in #55