In the default Dockerfile, `mix assets.deploy` downloads tailwind & esbuild every time assets change

[jyc]

Thanks for making Phoenix! I noticed that in the default Dockerfile mix assets.deploy downloads tailwind & esbuild every time assets change:

phoenix/priv/templates/phx.gen.release/Dockerfile.eex

Lines 46 to 55 in 69685f7

	RUN mix deps.compile
	COPY priv priv
	COPY lib lib
	<%= if assets_dir_exists? do %>
	COPY assets assets
	# compile assets
	RUN mix assets.deploy

Do you think the default could be changed to run

mix tailwind.install
mix esbuild.install

... after mix deps.compile and before mix assets.deploy? This should make most Docker image builds faster because they won't have to download tailwind/esbuild every time anything in priv, lib, or assets changes.

I'd be happy to open a small PR to do this if it's alright with you!

The only challenge is that it'd have to handle --no-tailwind and --no-esbuild. Happy to take a stab at handling that too with some pointers; a cursory search for no-tailwind didn't turn up anything: https://github.com/search?q=repo%3Aphoenixframework%2Fphoenix+no-tailwind&type=code

[SteffenDE]

@jyc I think it should be possible to add mix assets.setup instead of adding the tailwind and esbuild installs separately. We'd only need to check if the assets.setup task is actually available when generating the Dockerfile:

RUN mix deps.compile
<%= if @assets_setup_available? do %>mix assets.setup<% end %>

and adding the binding here:

phoenix/lib/mix/tasks/phx.gen.release.ex

Lines 246 to 250 in c848a84

	Keyword.merge(binding,
	debian: @debian,
	debian_vsn: debian_vsn,
	elixir_vsn: elixir_vsn,
	otp_vsn: otp_vsn

            elixir_vsn: elixir_vsn,
+           otp_vsn: otp_vsn,
+           assets_setup_available?: Keyword.has_key?(Mix.Project.config()[:aliases], :"assets.setup")

If you want to give it a go and send a PR, I'll be happy to review it.

[rhcarvalho]

I do an explicit mix assets.setup after mix deps.compile in a Dockerfile, can confirm it leads to better caching.

[josevalim]

LGTM! Can you folks please submit a PR? :)

[jyc]

Sorry for the delay! Thanks @SteffenDE for the guidance—I could make a PR but I realized I'm not sure how best to test this! I don't want to make the PR only for you to have to debug it, so please don't block on me! But let me know what you want to do, I'm also happy to learn how to debug it.

[rhcarvalho]

I went ahead and sent #5865, thanks @jyc!

I wanted to share the steps I took.

1. Identify existing tests: looked for "--docker" as the command is mix phx.gen.release --docker, and found test/mix/tasks/phx.gen.release_test.exs.
2. Run existing tests: I decided to run just the test file above with mix test test/mix/tasks/phx.gen.release_test.exs
3. Since there were already two tests covering with/without assets, all I needed to do was update them with the new expectations (have and not have the RUN mix assets.setup line).
4. See the tests fail: this gives us confidence that the tests actually work.
5. Implement the change in the template.
6. See the tests pass.

I have also done a manual test with and without assets to inspect the Dockerfile to ensure the formatting was okay (line breaks in the proper places and not too many or too few).

Then submitted the PR.

--

Now I realize I missed @SteffenDE's idea to check if the assets.setup task is actually available... Let's discuss on the PR, as we don't do that check for any other task in the Dockerfile -- e.g. we assume assets.deploy exist.

[jyc]

Wow! Thanks for making the PR and for documenting the process in detail @rhcarvalho! I really appreciate it.

[rhcarvalho]

@jyc 💜💜💜

Thank you for contributing with this well reported issue and looking forward to seeing your next contributions!