Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

IMalyugin
Copy link
Contributor

@IMalyugin IMalyugin commented Jun 26, 2023

Same check list :)

  • Verify that locally published package works
  • Verify that ctrl+c stops babel-watch
  • Verify that underlying app crash is handled properly
  • Verify auto restart after changing files works
  • Verify manual restart via "rs" works

@IMalyugin
Copy link
Contributor Author

Ready for review and merge on my side

@STRML
Copy link
Collaborator

STRML commented Jun 26, 2023

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
Copy link
Contributor Author

IMalyugin commented Jun 26, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

IMalyugin commented Jun 29, 2023

@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:

  • local issue solved with a local patch;
  • async-proof implementation;

Pitfalls:

  • any other error, that may occurs in message handler will be ignored;
  • extra scenario added to the workflow (handling unexpected crash in the callback);

@STRML
Copy link
Collaborator

STRML commented Jun 29, 2023

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

@STRML STRML merged commit bdcaa3e into kmagiera:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants