class-transformer
class-transformer copied to clipboard
typestack
Improve behavior around external errors
See my comments on this issue for additional contex: https://github.com/typestack/class-transformer/issues/174
Here's my attempt at improving the way errors thrown by external code are handled. I believe class-transformer should not attempt to suppress the errors that get thrown, but because of the metadata it records, it's in a privileged position to provide valuable error logging when a problem is encountered, so it seems like a waste to not log something.
I've added some tests, but they probably aren't that great. They end up printing errors to the console, which is exactly what I'd want to happen when I'm using the library and something goes wrong, but is pretty disruptive when running tests to validate a change to this repository.
I'm open to suggestions on how to make this better, the repo doesn't really have a precedent for error handling or logging, and this PR may end up setting that precedent if it's merged.
The biggest point I'm unsure of is whether we should throw a new error (like I currently am), or just log our custom info and re-throw the error we caught. I think re-throwing the original error is probably more transparent for any projects that already depend on this and might have some error-based control flow in place already, but throwing our own error doesn't necessarily break those use cases, and gives more valuable information in the error that surfaces in the users code.
I also think there would be some wisdom in adding a way to enable/disable the logging, so users can choose to suppress the console logs in production builds. This could be added as a simple boolean option in ClassTransformOptions, but I think it would be better to control something like that with a global setting somehow, rather than needing to add a parameter to every single function call. I don't think there's a mechanism in place for global settings like that, and it'd be a large addition, so I opted to leave it out for now.
Hi, @YGKtech thanks for your work, don't you mind if this feature will be added into next upcoming release after 0.2.0 will come out, there is already a lot of new stuff and fixing bugs commits, and I really want your solution, but after this one.
Thanks for your time, I really appreciate that.
@cojack : that's fine, this change probably only impacts a small group of users, and doesn't actually "fix" anything, so it makes sense to put it on hold while other fixes are prioritized.
What is the state of this branch? Is this feature ready? are any breaking changes?
@MTschannett : The feature is ready, and there are no breaking changes. The code behaves the same as it did before, it just logs some additional information when it encounters certain errors.
I believe I've addressed the changes @NoNameProvided requested, just waiting on him to sign off on it.
Ok. I see. I think we should wait a few days, otherwise I'll try to get this merged.
