kmagiera / babel-watch

Reload your babel-node app on JS source file changes. And do it fast.
MIT License
529 stars 70 forks source link

Fix: Add error forwarding from babel to stderr (#143) #144

Closed IMalyugin closed 1 year ago

IMalyugin commented 1 year ago

Same check list :)

IMalyugin commented 1 year ago

Ready for review and merge on my side

STRML commented 1 year ago

How does this actually work? The try/catch isn't going to cover the callback, so it's only effectively covering this line:

https://github.com/kmagiera/babel-watch/pull/144/files#diff-3ff4a3ee3a0bfeb4fbf652e6a71273903130d817040ffac875ac6540b98e5907R432

IMalyugin commented 1 year ago

It actually covers the handleFileLoad, that contains compile call.

I chose to catch as much as possible - so that any error thrown inside message handler gets handled. The rest is up to call stack propagation (or promise chain if there is async action going on).

Not sure if callback is covered, depends on whether babel.transformFile is synchronous (which is probably so)

IMalyugin commented 1 year ago

Might as well reduce the coverage to that one babel OptionManager.init call, depends on your liking really. Either we catch all errors possible or we catch errors known to exist

IMalyugin commented 1 year ago

@STRML Shall we get back to the matter at hand?

I can move try/catch closer to the Babel initialization and send the error over callback, if you think that's best.

Benefits:

Pitfalls:

STRML commented 1 year ago

I'm going to merge this, but also fix the async issue I highlighted here. Thanks for the contribution.