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

compileToStringSync returns empty string on error #110

Merged

Conversation

omnibs
Copy link
Contributor

@omnibs omnibs commented Apr 8, 2021

I modified https://github.com/phenax/esbuild-plugin-elm/ to use compileToStringSync and noticed our tools stopped being able to tell when Elm compilation failed.

Traced it back to us not handling the compiler process exit status on compileToStringSync.

Copy link
Owner

@rtfeldman rtfeldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thank you! 🤘

@rtfeldman rtfeldman merged commit 5801685 into rtfeldman:master Apr 8, 2021
@rtfeldman rtfeldman deleted the fix-compile-to-string-sync-errors branch April 8, 2021 21:52
@rtfeldman
Copy link
Owner

Published to npm as 5.0.6

rogeriochaves added a commit to rogeriochaves/esbuild-plugin-elm that referenced this pull request Jun 19, 2022
I want to be able to display the error message on the screen, for that I need to capture the error message produced during a compilation error, but this is not possible because the sync function from node-elm-compiler returns just a hardcoded string, as you can see here: rtfeldman/node-elm-compiler#110

Since we are already in an async function, we might as well just use await here and get the proper error message
@rogeriochaves
Copy link

Hey there, but why returning a hardcoded error string? Shouldn't we return the full error message? I needed that so I opened a pull request in esbuild-plugin-elm as well, but maybe the change should be made here?

phenax/esbuild-plugin-elm#20

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

3 participants