Skip to content

Comments

convert inject/1 to a macro#7

Open
levine1726 wants to merge 2 commits intosonerdy:masterfrom
levine1726:convert-inject-to-macro-so-get-env-is-called-only-when-configured-to
Open

convert inject/1 to a macro#7
levine1726 wants to merge 2 commits intosonerdy:masterfrom
levine1726:convert-inject-to-macro-so-get-env-is-called-only-when-configured-to

Conversation

@levine1726
Copy link

@levine1726 levine1726 commented Sep 28, 2022

this ensures that Application.get_env is called only when inject is enabled in the config. Otherwise, it does not get included in the compiled code.

Tested by putting this branch in an existing project using inject. Confirmed that tests passed there, and that app code in development worked. Also ran mix test against this repo.

lib/inject.ex Outdated
Comment on lines 11 to 12
defmacro inject(mod) do
quote bind_quoted: [mod: mod] do
Copy link
Author

@levine1726 levine1726 Sep 28, 2022

Choose a reason for hiding this comment

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

Should I add a guard clause that ensures mod is an atom and not an AST tuple?

|> find_override() || mod
end
end
end
Copy link

Choose a reason for hiding this comment

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

The best way to visualise macros (well, for me at least) is to think of them as copy & paste.
With macro defined like this we would get this code:

def fancy do
  i(Dep).fun()
end

replaced with

def fancy do
  (
      if Application.get_env(:inject, :disabled) do
         mod
       else
         Inject.Registry
         |> Registry.lookup(mod)
         |> Enum.reverse()
         |> find_override() || mod
       end
  ).fun()
end

This would still call the Application.get_env every time fancy/0 is called.

What we want instead, is to evaluate the Application.get_env call in compile time, inside macro:

defmacro inject(mod) do
  # run stuff here, during compilation
  if letsdothis do
    quote(do: injectstuff)
  else
    quote(do: nah)
end

This way the body of fancy/0 would be either (injectsutff).fun() or (nah.fun()).

Now, what should injectsutff and nah be is not for me to decide ;)
But I'll give you one more hint - it's usually a good practice to put the least amount of code inside quote as possible. If you can replace some lines with a function call and just call that function inside quote it should be done.

Can't wait to see the v2! 🎸

Copy link
Author

Choose a reason for hiding this comment

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

I took a stab at V2. I just amended the commit; should have just squashed down before merging so you could see the change history but oh well, at least the change is small! I also asked a separate question about mod which you can see in the diff.

By the way, the test in test/inject_configuration_test.exs now fails, since changing the environment at runtime won't change the compiled code. Is there a way to recompile the code just for a test, or should we just remove this check? For what it's worth, I don't know if it's possible to do this from a quick google test.

@levine1726 levine1726 force-pushed the convert-inject-to-macro-so-get-env-is-called-only-when-configured-to branch 2 times, most recently from 289f7c5 to ca4babb Compare October 9, 2022 19:48
def inject(mod) do
defmacro inject(mod) do
if Application.get_env(:inject, :disabled) do
mod
Copy link
Author

Choose a reason for hiding this comment

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

Note that I didn't wrap this in a quote block. I can do it if needed, but I think it would be redundant since we're not modifying the incoming atom or AST (not that I would see inject/1 receiving an AST any time soon).

Copy link
Author

Choose a reason for hiding this comment

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

Also, doing quote(do: mod) here throws warnings and I'm not sure why.... 🤔

Copy link

Choose a reason for hiding this comment

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

Since quote is kinda like literal copy paste, what would be the result of such code?

this ensures that Application.get_env is called only when inject is
enabled in the config. Otherwise, it does not get included in the compiled code.
@levine1726 levine1726 force-pushed the convert-inject-to-macro-so-get-env-is-called-only-when-configured-to branch from ca4babb to 50fbd24 Compare October 9, 2022 20:10
|> Enum.reverse()
|> find_override() || mod
end
inject_module(mod)
Copy link

Choose a reason for hiding this comment

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

Almost there :)

Calling inject_module macro here would run the macro once during compilation and that's it. What we want is:

  • An inject_module function (not macro) that does the injection (always, unconditionally)
  • An inject macro, that based on application env config either does nothing or calls that inject_module function.

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.

2 participants