-
Notifications
You must be signed in to change notification settings - Fork 34
fix: implement handle_info/2 #94
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: implement handle_info/2 #94
Conversation
@grossvogel do you think you could have a look at this ? |
end | ||
|
||
{:noreply, state} | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of things that concern me about this change as-is:
- I believe the call to
Mix.env()
will raise an exception in any application shipped as a release, sinceMix
isn't included / started in releases - If the library needs to emit log messages, it should use
Logger
instead ofIO
so the authors of the application can have better control over where / whether those messages get logged
It seems like the big issue here is that your JS runtime is emitting extra messages on startup due to the debugger, which I think is just not foreseen or handled well by this library or generally by the stdin / stdout method of communication with the Port. Something like this might work, but if the JS app were to emit text to stdout during the execution of a function, you can see how that'd really mess with the receive
loop above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
I'll fix the mix.env() and the IO call.
For the rest, I don't understand what's your concern. These messages are currently not processed by the library, and we get runtime errors because the handle_info callback is not implemented. I'm just suppressing these errors without affecting at all how the library currently works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@francois-codes I was mainly thinking out loud, not arguing against your change in particular. I think your experience makes me feel the library is a bit more fragile than I'd like because there's only 1 channel of communication between the JS and the Elixir app, and it carries both things the app really needs to pay attention to and parse as well as a bunch of superfluous messages emitted by the JS runtime.
So I feel that adding a handle_info()
is OK as far as it goes, but ultimately it makes me want to put a disclaimer on the library that you may encounter difficulties if your JS code or runtime emits messages to stdout or stderr. The library may (if the timing is wrong) mistake those messages for part of the result of a function call, leading to unpredictable results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the IO call to use the logger, and guarded against Mix.env() crashing in release mode.
I think it's safe this way - again, these messages are not meant to be processed by the library.
Let me know if it's fine like this or if you'd like some other changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @francois-codes,
First, we sincerely apologize for the long wait on this PR. Our team is short-handed and focused on client work, making it difficult to give our open source projects the attention they deserve.
We're merging your PR as it addresses an important issue blocking projects like live_vue. While your solution with Mix.env() rescue works, there are some concerns with this approach in releases - custom release configurations might handle module loading differently, future Elixir versions could change Mix behavior, and there's a small performance hit from triggering rescues on every call.
We'll follow up with a small refactor to use Application configuration instead:
defp debug_mode? do
Application.get_env(:nodejs, :debug_mode, false)
end
This provides a more reliable approach that's consistent with Elixir best practices.
Thanks for your contribution and understanding regarding our response time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI #99
We will get a new release out once this gets in. Probably wont be until monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Dacello
Thank you for moving this along.
I’m sorry for expressing my frustration as bluntly as I did, but as you can see, it took almost 2 weeks to get a first review on this PR, and I’m not sure it would have happened at all if I hadn’t hunt down the maintainers on twitter to get that first review.
I replied to the review the following day, but after a very quick back and forth, this was left hanging again. And to be honest, I didn’t have the patience & energy to keep pushing for it.
I understand the challenge of maintaining an open source project and the fact that maintainers are doing this for free on their own time. I have seen many entitled requests from consumers of open source libraries just asking maintainers to fix things asap. I know this isn’t the way to go, and this isn’t what I was expecting, so I’m hoping this isn’t how my previous interactions came across.
I also understand and respect the fact that the maintainers have every right to disagree with a proposed change and refuse it, request it to be done differently, or simply say they don’t have the time to process the PR for the moment.
I would have preferred that over having no response at all.
Anyway, I forked the lib and used that in the meantime. After all, that’s also part of how open source works. I’m happy to see that you’ve now picked this up, and even started to improve upon it. I’d be glad to test things out and help if I can. I don’t intend to maintain a public fork either so if we can rely on this lib again, this is good news.
I’ll watch out for the release
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 @Dacello big thank you for your work and transparency! 👐
I found the same error and forking this lib and applying these changes solved it 👍 |
Thanks @andresgutgon it's a shame, but I had to resort to a fork too, as you can see from the history that it's almost impossible to get things moving on this repo. |
@francois-codes @andresgutgon FYI we just published v3.1.3 which contains this and #99 -- these should solve your problems. Let us know if you still have any issues. Thanks again for your patience. |
When running the project again, I noticed some errors I didn't see before in the output:
This messages correspond to the messages sent by the nodejs for warnings and debugging info.
This PR fixes the problem by implementing the
handle_info/2
method, as advised in the error message. In order to avoid the spam, I added a check forMix.env()
to make sure we were only showing this information in dev.This is the equivalent of the command above with this PR:
FYI @Valian @grossvogel