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

refactor: common PubSub functions + macro #244

Merged
merged 5 commits into from
Nov 21, 2024
Merged

Conversation

KaylaBrady
Copy link
Collaborator

Summary

Follow up to #243

What is this PR for?

This PR pulls out some common functions from our 3 realtime PubSub modules. I initially tried to do this entirely using macros, but pulling more into regular functions winded up being easier and I think easier to follow.

@KaylaBrady KaylaBrady requested a review from a team as a code owner November 20, 2024 20:33
@KaylaBrady KaylaBrady requested review from EmmaSimon and removed request for a team November 20, 2024 20:33
@KaylaBrady KaylaBrady added the deploy to dev-orange Automatically deploy this PR to dev-orange label Nov 20, 2024

def handle_info(:timed_broadcast, state) do
send(self(), :broadcast)
interval = unquote(broadcast_interval_ms)
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that this unquote works, since broadcast_interval_ms is not a variable within the scope of __using__/1 - I'd expect a bare broadcast_interval_ms to work here, although storing broadcast_interval_ms as a module attribute @broadcast_interval_ms might be closer to what we'd write by hand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried removing unquote but it doesn't work. Isn't broadcast_interval_ms defined as a variable in the scope of __using__/1/ here?

I like the approach of passing it as a param so that it helps to direct a reader to see that the interval-based broadcasting logic is done by the macro, rather than having to otherwise search for usage of @broadcast_interval_ms

Copy link
Member

Choose a reason for hiding this comment

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

That line defines broadcast_interval_ms as a variable within the body of the quote block, so I'd expect it to be usable (aside from macro hygiene) in the modules that call use MobileAppBackend.PubSub, but not in the __using__ macro.

I'd expect an error like this:

iex(1)> quote do
...(1)>   x = 3
...(1)>   unquote(x)
...(1)> end
error: undefined variable "x"
  iex:3

I think the reason this isn't happening is that bind_quoted automatically disables unquote in the quote call itself, so when use MobileAppBackend.PubSub is expanded, it expands into

defmodule Foo do
  broadcast_interval_ms = Keyword.fetch!([broadcast_interval_ms: ...], :broadcast_interval_ms)
  def handle_info(:timed_broadcast, state) do
    interval = unquote(broadcast_interval_ms)
  end
end

and since def is a macro itself the unquote just kinda works and finds the variable within the module body.

The fact that the module-level variable is referenced with unquote is really counterintuitive, though, and we'd never write code like the above snippet. If we were writing similar code by hand, we'd be far more likely to write

defmodule Foo do
  @broadcast_interval_ms Keyword.fetch!([broadcast_interval_ms: ...], :broadcast_interval_ms)
  def handle_info(:timed_broadcast, state) do
    interval = @broadcast_interval_ms
  end
end

which should be easy to generate with

defmacro __using__(opts) do
  quote location: :keep, bind_quoted: [opts: opts] do
    use GenServer

    @broadcast_interval_ms Keyword.fetch!(opts, :broadcast_interval_ms)

    # ...

    def handle_info(:timed_broadcast, state) do
      interval = @broadcast_interval_ms
    end
  end
end

which is what I meant when I suggested storing broadcast_interval_ms in a module attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah thanks for the explanation, I misunderstood your initial suggestion. Will update!

lib/mobile_app_backend/predictions/pub_sub.ex Outdated Show resolved Hide resolved
@KaylaBrady KaylaBrady merged commit aa4cebb into main Nov 21, 2024
5 checks passed
@KaylaBrady KaylaBrady deleted the kb-pubsub-refactor branch November 21, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy to dev-orange Automatically deploy this PR to dev-orange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants