nativew / esbuild-plugin-babel

Babel plugin for esbuild.
ISC License
72 stars 14 forks source link

Fix error in callback handling #1

Closed delanni closed 3 years ago

delanni commented 3 years ago

Hey!

I found that the code handling the babel transformation, which is basically the key of the plugin might be flawed.

In the original code:

// snippet from line :19
babel.transform(contents, babelOptions, (error, result) => {
  if (error) throw error;
  contents = result.code;
});

return { contents };

This original code would return before the callback would be ever fired, unless the implementation for babel.transform does call the callback synchronously, which I believe it's not doing.

Fixing this locally also unblocked my babel compilation step, so I think it might be a valid fix.

Please check my PR for a suggestion on fixing.

hum-n commented 3 years ago

Thanks! I merged most of it in 0.2.1. I added await to the returns. The else is unnecessary since there's a return.

delanni commented 3 years ago

I'm well aware that else are not mandatory, but I consider it a good practise, it makes code a bit more readable compared to single line ternaries.

Also, "awaiting" promises before returning is also unnecessary. Async functions can return values or promises, any promise returned would be resolved before the next handler is called.

But it's your code 🤷 you make it ugly the way you prefer.

hum-n commented 3 years ago

I don't agree that else is more readable. I can understand for ternaries though. Thanks for the await tip, I'll remove them.