Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Fix error in callback handling #1

Closed

Conversation

delanni
Copy link
Contributor

@delanni delanni commented Feb 10, 2021

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
Copy link
Member

hum-n commented Feb 12, 2021

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.

@hum-n hum-n closed this Feb 12, 2021
@delanni
Copy link
Contributor Author

delanni commented Feb 12, 2021

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
Copy link
Member

hum-n commented Feb 12, 2021

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants