Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 16, 2025
+16 −0
Merged

fix: implement handle_info/2 #94

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f63c61
fix: implement handle_info/2
francois-codes Dec 26, 2024
42b3c81
fix: use logger and guard agaisnt mix.env in release
francois-codes Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lib/nodejs/worker.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ defmodule NodeJS.Worker do
@moduledoc """
A genserver that controls the starting of the node service
"""
require Logger

@doc """
Starts the Supervisor and underlying node service.
Expand Down Expand Up @@ -123,6 +124,21 @@ defmodule NodeJS.Worker do
end
end

defp env do
Mix.env()
rescue
_ -> :release
end

def handle_info({_pid, data}, state) do
with :dev <- env(),
{_, {:eol, msg}} <- data do
Logger.info("NodeJS: #{msg}")
end

{:noreply, state}
end
Copy link
Contributor

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, since Mix isn't included / started in releases
  • If the library needs to emit log messages, it should use Logger instead of IO 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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Member

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!

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor

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! 👐


defp decode(data) do
data
|> to_string()
Expand Down