-
Notifications
You must be signed in to change notification settings - Fork 1k
Upgrade :esbuild to minimum version when upgrading to 1.1 #4011
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
Conversation
igniter, | ||
igniter | ||
|> Igniter.Project.Deps.add_dep( | ||
{:esbuild, "~> 0.10", runtime: quote(do: Mix.env() == :dev)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, there is a small issue.
Usually, in a freshly generated Phoenix project, we have a line for :esbuild
like below in mix.exs
for the dependencies:
{:esbuild, "~> 0.8", runtime: Mix.env() == :dev},
Why quote
?
quote
is used so that it generates runtime: Mix.env() == :dev
. Without it, it would generate something like runtime: false
.
Consequence of using quote
At the upgrade confirmation, it prints an AST instead of Mix.env() == :dev
:
Desired: `{:esbuild, "~> 0.10", [runtime: {:==, [context: Phoenix.LiveView.Igniter.UpgradeTo1_1, imports: [{2, Kernel}]], [{{:., [], [{:__aliases__, [alias: false], [:Mix]}, :env]}, [], []}, :dev]}]}`
Found: `{:esbuild, "~> 0.8", [runtime: true]}`
This might not be so clear for the user...
Alternative to quote
An alternative would be to just add a raw value runtime: :dev
, it would look like this:
|> Igniter.Project.Deps.add_dep(
{:esbuild, "~> 0.10", runtime: :dev},
)
But this would then upgrade AND update the value for :runtime
, which might not be ideal, because we just need to upgrade the version without touching the test.
I would need some guidance on this one, because both has some drawbacks. I have chosen the one that has an unclear message, but without modifying something that we should not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Ash team me proposed two alternatives to solve this solutions:
- Pass
:yes?
toadd_dep
, so that "Desired" and "Found" are not shown and the upgrade is directly done. (discord answer) - Move the quoted value into
set_dep_option/4
(discord answer)
I have chosen solution 2. so that it is clearer what the changes are going to happen.
Here the printed result when solution 2. is used:
Desired: `{:esbuild, "~> 0.10"}`
Found: `{:esbuild, "~> 0.8", [runtime: true]}`
The AST is not printed anymore and the versions are shown correctly. A small drawback is that it is not exactly like what is written in mix.exs
, because runtime: Mix.env() == :dev
is missing.
If this solution is not fine for you, I will make changes for solution 1.
Thanks for the Ash team for their support.
Related issue on Igniter: ash-project/igniter#338
Nice! Thank you :) |
* Upgrade :esbuild to minimum version when upgrading to 1.1 * Pass quoted code to set_dep_option/4
This PR is a proposal to upgrade
:esbuild
to the minimum version0.10
while running Igniter in order to avoid errors on joining environment variables.Please check my main concern about showing an unclear message because of the usage of
quote
.