Skip to content
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

Igniter task to install Phoenix #140

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

leandrocp
Copy link
Contributor

@leandrocp leandrocp commented Nov 3, 2024

Hey @zachdaniel here's the initial draft for igniter.install_phoenix. In short it does mimic phx.new.ex by wrapping generator.ex and single.ex to include the Igniter pipeline. With this approach we can reuse the same template files and reuse some functions, for eg Single.render/3, and also allows us to gradually migrate the generator to use Igniter functions.

Before moving forward, I'd like some feedback and review of the current code (see the comments).

To test this PR:

mix new my_app --sup
# add
{:igniter, path: "/path/to/igniter"}
mix deps.get
mix igniter.install_phoenix .

""
end

!String.valid?(Rewrite.Source.get(source, :content)) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some files do not have a valid string content, for eg binary files like favicon.ico but I think we still need to display that such file will be created. Might need to improve this code to detect updates as well.

lib/igniter.ex Outdated Show resolved Hide resolved
mix.exs Outdated
@@ -100,6 +100,7 @@ defmodule Igniter.MixProject do
{:spitfire, "~> 0.1 and >= 0.1.3"},
{:sourceror, "~> 1.4"},
{:jason, "~> 1.4"},
{:phx_new, "~> 1.7", runtime: false},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 This one is tough. I don't actually think we need this. IIRC phx_new will be available as long as the user has the archive installed? Which could mean that we can just at the start say "please install phx_new to use this task" if the module is not defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we could have it as a test only dependency in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't actually think we need this. IIRC phx_new will be available as long as the user has the archive installed? Which could mean that we can just at the start say "please install phx_new to use this task" if the module is not defined.

Should we have it as an optional dependency, in that case? That seems like the best of both worlds: It doesn't become a transient dependency that all apps using Igniter are required to fetch, but it still allows Igniter to test against it and specify a version spec that Igniter's compatible with.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I think that would make sense, yes. Since we'd be switching in our code on the module being compiled, it wouldn't matter if it's from a dep or the archive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to optional: true and added a check. Tested in a project, it will ask to install phx_new if not available.

@zachallaun
Copy link
Collaborator

This is awesome! Having not looked at the code yet, a couple quick thoughts:

  1. Will --sup be required to start a new project this way? Is it possible for the installer to set up the supervision tree itself if it's not present?
  2. What about mix igniter.phx.install? I imagine there will be additional Phoenix-related tasks in the future, so it might be worth "namespacing" it from the onset.

@leandrocp
Copy link
Contributor Author

This is awesome! Having not looked at the code yet, a couple quick thoughts:

  1. Will --sup be required to start a new project this way? Is it possible for the installer to set up the supervision tree itself if it's not present?

Not required. You can execute the task in any project, the files are relative to the root path. So that can potentially be used to upgrade Phoenix in your app for example.

  1. What about mix igniter.phx.install? I imagine there will be additional Phoenix-related tasks in the future, so it might be worth "namespacing" it from the onset.

I was thinking about it and forgot to propose in this PR 😂
But that was my reasoning as well, I agree mix igniter.phx.install makes more sense.

- make :phx_new optional and check if it's loaded
- add remaining user-facing opts
- require Elixir ~> 1.15
- validate %Project{}
- use Phoenix extension
@leandrocp leandrocp changed the title Mix task to install Phoenix Igniter task to install Phoenix Nov 6, 2024
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.

3 participants