Skip to content

Use umbrella apps names instead of "default"#5683

Merged
josevalim merged 1 commit intophoenixframework:mainfrom
agonzalezro:no-default-in-umbrella-configs
Dec 30, 2023
Merged

Use umbrella apps names instead of "default"#5683
josevalim merged 1 commit intophoenixframework:mainfrom
agonzalezro:no-default-in-umbrella-configs

Conversation

@agonzalezro
Copy link
Contributor

@agonzalezro agonzalezro commented Dec 28, 2023

When adding more than one app to your umbrella project you can't use default for tailwind and esbuild configurations or they will clash between them. By using their names instead of "default" the issue is fixed.

Closes #5631

Steps to test it (assuming you are using phoenix mix from source code):

  1. mix phx.new --umbrella /tmp/a
  2. mix phx.new.web /tmp/a_umbrella/apps/b
  3. Change config/runtime.exs & config/dev.exs to change one of the apps to listen in something that isn't 4000.
  4. cd /tmp/a_umbrella && mix ecto.migrate

@josevalim
Copy link
Member

We also need to change mix.exs. I would actually go ahead and change this for regular apps as well. :)

config :esbuild,
version: "0.17.11",
default: [
"<%= @web_app_name %>": [
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need the quotes, the web app name should be a valid identifier.

@agonzalezro agonzalezro force-pushed the no-default-in-umbrella-configs branch from 4540c6e to dc982c8 Compare December 28, 2023 19:14
@agonzalezro
Copy link
Contributor Author

@josevalim I've changed it for single apps as well and remove the extra quotes. However, I am not sure what needs to be done in mix.exs.

@josevalim
Copy link
Member

The aliases like assets.setup need to pass the namespace now, otherwise they default to “default”, no? You can generate the app and give it a try!

@agonzalezro
Copy link
Contributor Author

Understood @josevalim, fixed with the last commit. Thanks for the heads up!

@agonzalezro
Copy link
Contributor Author

The tests are failing because of the formatter, example:

❯ mix do format --check-formatted
...
** (Mix) mix format failed due to --check-formatted.
The following files are not formatted:

/private/tmp/w/mix.exs

       |
75 75  |      "assets.setup": ["tailwind.install --if-missing", "esbuild.install --if-missing"],
76 76  |      "assets.build": ["tailwind w", "esbuild w"],
77    -|      "assets.deploy":
78    -|        ["tailwind w --minify", "esbuild w --minify", "phx.digest"]
   77 +|      "assets.deploy": ["tailwind w --minify", "esbuild w --minify", "phx.digest"]
79 78  |    ]
80 79  |  end
       |

The problem I see now is that either with inspect or IO.puts I would need to mimic what the formatter does. Is there any reason why the output of the template generation isn't being formatted as another step?

@josevalim
Copy link
Member

You can always break them over multiple lines and the formatter will always be happy with it. :)

@agonzalezro agonzalezro force-pushed the no-default-in-umbrella-configs branch 3 times, most recently from f3cc0eb to 7fb900a Compare December 30, 2023 15:22
When adding more than one app to your umbrella project you can't use default
for tailwind and esbuild configurations or they will clash between them. By
using their names instead of "default" the issue is fixed.

This change also changes the behaviour for single apps.

To make the formatter happy the commands need to be splitted in several lines.

Closes phoenixframework#5631
@agonzalezro agonzalezro force-pushed the no-default-in-umbrella-configs branch from 7fb900a to 40de999 Compare December 30, 2023 15:30
@agonzalezro
Copy link
Contributor Author

Could the faulty test be flaky? I can't rerun, maybe it would fix it?

Also, I am not extremely happy about how I solved the multi-line, any suggestion would be more than welcomed!

Thanks 🙏🏽

@josevalim josevalim merged commit 9f24517 into phoenixframework:main Dec 30, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

studzien pushed a commit to Whatnot-Inc/phoenix that referenced this pull request Feb 8, 2024
When adding more than one app to your umbrella project you can't use default
for tailwind and esbuild configurations or they will clash between them. By
using their names instead of "default" the issue is fixed.

To make the formatter happy the commands need to be splitted in several lines.

Closes phoenixframework#5631
studzien pushed a commit to Whatnot-Inc/phoenix that referenced this pull request Feb 16, 2024
When adding more than one app to your umbrella project you can't use default
for tailwind and esbuild configurations or they will clash between them. By
using their names instead of "default" the issue is fixed.

To make the formatter happy the commands need to be splitted in several lines.

Closes phoenixframework#5631
studzien pushed a commit to Whatnot-Inc/phoenix that referenced this pull request Feb 20, 2024
When adding more than one app to your umbrella project you can't use default
for tailwind and esbuild configurations or they will clash between them. By
using their names instead of "default" the issue is fixed.

To make the formatter happy the commands need to be splitted in several lines.

Closes phoenixframework#5631
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.

Multiple web apps in umbrella project generate conflicting esbuild and tailwind configs

2 participants