phoenixframework / tailwind

An installer for tailwind
MIT License
473 stars 60 forks source link

It allows downloading a custom binary. #60

Closed paridin closed 1 year ago

paridin commented 1 year ago

The idea is to reuse the tailwind project to download a custom binary, which could use another convention name for the URL. #59

config :tailwind,
  version: "3.1.8",
  download_fn: fn version, target ->
    "https://storage.defdo.de/tailwind_cli_daisyui/v#{version}/tailwindcss-#{target}"
  end

image

image

petermm commented 1 year ago

fyi - config doesn't support functions:

#27 15.10 * assembling myapp-0.1.0 on MIX_ENV=prod
#27 15.12 * using config/runtime.exs to configure the release at runtime
#27 15.78 ** (Mix) Could not read configuration file. It has invalid configuration terms such as functions, references, and pids. Please make sure your configuration is made of numbers, atoms, strings, maps, tuples and lists. The following entries are wrong:
#27 15.78 
#27 15.78 Application: :tailwind
#27 15.78 Key: :download_fn
#27 15.78 Value: #Function<41.3316493/2 in :erl_eval.expr/6>
#27 15.78 
------

maybe something more along the lines of: https://github.com/phoenixframework/tailwind/compare/master...petermm:tailwind:custom_base_url

paridin commented 1 year ago

Hi @petermm

Thanks for your review; I didn't check to use it on the runtime config; could you try something like this?

# config/runtime.exs
defmodule CustomURL do
   def build_url(version, target) do
     "https://storage.defdo.de/tailwind_cli_daisyui/v#{version}/tailwindcss-#{target}"
   end
end

config :tailwind,
  version: "3.1.8",
  download_fn: &CustomURL.build_url/2

I will check in detail tomorrow.

paridin commented 1 year ago

@petermm I just adjusted the PR to keep it simple doing the name assumption, as a rule, I think it should be ok, thanks for reviewing.

chulkilee commented 1 year ago

@paridin Using predefined URL structure is not helpful here especially when using other sites :)

For flexibility - you can pass {module, func_name} tuple via config. Here is an idea:

paridin commented 1 year ago

@chulkilee That sound like a good idea. Let's see if the new commit fits better.

josevalim commented 1 year ago

Thanks but in a team, multiple people will have different machines. We should rather allow mix tailwind.install PATH/TO/URL so those with custom needs can point to the desired URL.

paridin commented 1 year ago

Thanks but in a team, multiple people will have different machines. We should rather allow mix tailwind.install PATH/TO/URL so those with custom needs can point to the desired URL.

Here is my proposal

When the team provides the base URL, we will concatenate for the last part of the URL a standard binary name that includes the version.

mix tailwind.install https://domain.com/tailwind_cli_daisyui/

Detecting that execution, we will append the standard binary name resulting in:

https://domain.com/tailwind_cli_daisyui/v3.1.8/tailwindcss-macos-x64

Another possibility is for the team to provide the URL with the binary, and it includes the architecture in this case we skip to append the standard name.

mix tailwind.install https://domain.com/v3.1.8/tailwindcss-macos-x64

And the last option in mind is to add a new flag to skip appending the standard name to the URL.

mix tailwind.install https://domain.com/tailwind-non-standard-binary-name --skip-append-path

Doing those operations, it should be flexible to decide how to proceed to download the custom binary.

What do you think?

josevalim commented 1 year ago

That’s a solid proposal. Instead of —skip-append-path we can also look off the URI finishes with / or not. :)

paridin commented 1 year ago

Sure, I handle it, thanks for your suggestion.

josevalim commented 1 year ago

Please ping me once it is ready for another look!

paridin commented 1 year ago

@josevalim It's ready. I guess checking if the slash at the end exists should work for all cases, but to ensure, I added the last verification to append the standard name only when the URL does not contain an available target.

What do you think?

josevalim commented 1 year ago

@paridin if that is the case, maybe it is better to be explicit and add $target and $version to the URL? So we do:

base_url |> String.replace("$target", target()) |> String.replace("$version", version())

I think this way we can unify the code paths and even write the default url using $target and $version. WDYT?

josevalim commented 1 year ago

The nice thing about $version and $target is that if you just want to hardcode, that works too, and we don't need special logic for it.

paridin commented 1 year ago

So the main idea is: When the team wants to download a custom binary, it must provide the replaceable version or target to be filled by internals, and it must provide the full path, which means drop the conditionals, proper?

Then the mix task will be:

# Hard coding the URL
mix tailwind.install https://domain.com/v3.2.4/tailwindcss-macos-x64

# version filled by internals
mix tailwind.install https://domain.com/v$version/tailwindcss-macos-x64

# version and target filled by internals
mix tailwind.install https://domain.com/v$version/$target

# target filled by internals
mix tailwind.install https://domain.com/non-version/$target

Should we remove the flow to append the rest of the path when the URL ends with a slash, or we preserve it?

mix tailwind.install https://domain.com/custom-binary/

It sounds like a simplified behavior, which is excellent to me, too; let me know if I'm on the same path as your idea to commit to those changes.

josevalim commented 1 year ago

Yes, exactly. We can remove the flow to append the rest too!

paridin commented 1 year ago

@josevalim I think it is done! Thanks for the review.

josevalim commented 1 year ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: