Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions installer/lib/phx_new/generator.ex
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,12 @@ defmodule Phx.New.Generator do
# rules specific to new apps
@new_project_rules_files["project.md"],
@new_project_rules_files["phoenix.md"],
project.binding[:html] && @new_project_rules_files["phoenix-ui.md"],
project.binding[:html] && @new_project_rules_files["phoenix-html.md"],
project.binding[:live] && @new_project_rules_files["phoenix-live.md"],
# --no-assets is equivalent to --no-tailwind && --no-esbuild;
# we check for both here
project.binding[:javascript] && project.binding[:css] &&
# Only include assets.md for HTML projects (not API-only)
project.binding[:html] && project.binding[:javascript] && project.binding[:css] &&
@new_project_rules_files["assets.md"],
# generic usage rules
"\n<!-- usage-rules-start -->",
Expand Down Expand Up @@ -157,6 +160,20 @@ defmodule Phx.New.Generator do
@rules_files["liveview.md"],
"\n<!-- phoenix:liveview-end -->"
],
# Include ecto-forms when ecto and html are present, but NOT live
project.binding[:ecto] && project.binding[:html] && !project.binding[:live] &&
[
"<!-- phoenix:ecto-forms-start -->\n",
@rules_files["ecto-forms.md"],
"\n<!-- phoenix:ecto-forms-end -->"
],
# Include ecto-live-forms when both ecto and live are present
project.binding[:ecto] && project.binding[:live] &&
[
"<!-- phoenix:ecto-live-forms-start -->\n",
@rules_files["ecto-live-forms.md"],
"\n<!-- phoenix:ecto-live-forms-end -->"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

While this works for phx.new, it doesn't work for mix usage_rules.sync, since you then get basically the same content twice :(

So I don't see a good way to make this work without lots of duplication (basically having one set of rules for the root usage-rules and then a separate set of files for the generator). The question then is if we're fine with that and we want to strive for perfect AGENTS.md for all possible phx.new flags, or if we instead explicitly only support the default case. Then, we'd tell people to use --no-agents-md or we could also skip generating the AGENTS.md file for non-trivial flags to not generate confusing content. I don't know... (cc @chrismccord @josevalim)

Copy link
Member

Choose a reason for hiding this comment

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

@SteffenDE I think you can control in usage_rules what you want to sync, no?

In any case, I think this is changing too much at the same time. For example, I would not be inclined to split the Phoenix one into multiple sections. But I am fine with two versions for Ecto (as long as we confirm usage_rules can choose what to sync).

So in my two cents:

  • changes to elixir.md is good
  • changes to ecto.md is good
  • having two Ecto guides is fine (as long as we confirm usage_rules can be customised)

"<!-- usage-rules-end -->"
]
|> Enum.reject(fn part -> part == nil or part == false end)
Expand Down
6 changes: 6 additions & 0 deletions installer/templates/usage-rules/phoenix-html.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
### Phoenix Components

- Out of the box, `core_components.ex` imports an `<.icon name="hero-x-mark" class="w-5 h-5"/>` component for hero icons. **Always** use the `<.icon>` component for icons, **never** use `Heroicons` modules or similar
- **Always** use the imported `<.input>` component for form inputs from `core_components.ex` when available. `<.input>` is imported and using it will save steps and prevent errors
- If you override the default input classes (`<.input class="myclass px-2 py-1 rounded-lg">)`) class with your own values, no default classes are inherited, so your
custom classes must fully style the input
6 changes: 6 additions & 0 deletions installer/templates/usage-rules/phoenix-live.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
### Phoenix LiveView Layout Guidelines

- **Always** begin your LiveView templates with `<Layouts.app flash={@flash} ...>` which wraps all inner content
- Anytime you run into errors with no `current_scope` assign:
- You failed to follow the Authenticated Routes guidelines, or you failed to pass `current_scope` to `<Layouts.app>`
- **Always** fix the `current_scope` error by moving your routes to the proper `live_session` and ensure you pass `current_scope` as needed
4 changes: 4 additions & 0 deletions installer/templates/usage-rules/phoenix-ui.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Phoenix Layouts & Flash

- The `MyAppWeb.Layouts` module is aliased in the `my_app_web.ex` file, so you can use it without needing to alias it again
- Phoenix v1.8 moved the `<.flash_group>` component to the `Layouts` module. You are **forbidden** from calling `<.flash_group>` outside of the `layouts.ex` module
11 changes: 1 addition & 10 deletions installer/templates/usage-rules/phoenix.md
Original file line number Diff line number Diff line change
@@ -1,12 +1,3 @@
### Phoenix v1.8 guidelines

- **Always** begin your LiveView templates with `<Layouts.app flash={@flash} ...>` which wraps all inner content
- The `MyAppWeb.Layouts` module is aliased in the `my_app_web.ex` file, so you can use it without needing to alias it again
- Anytime you run into errors with no `current_scope` assign:
- You failed to follow the Authenticated Routes guidelines, or you failed to pass `current_scope` to `<Layouts.app>`
- **Always** fix the `current_scope` error by moving your routes to the proper `live_session` and ensure you pass `current_scope` as needed
- Phoenix v1.8 moved the `<.flash_group>` component to the `Layouts` module. You are **forbidden** from calling `<.flash_group>` outside of the `layouts.ex` module
- Out of the box, `core_components.ex` imports an `<.icon name="hero-x-mark" class="w-5 h-5"/>` component for hero icons. **Always** use the `<.icon>` component for icons, **never** use `Heroicons` modules or similar
- **Always** use the imported `<.input>` component for form inputs from `core_components.ex` when available. `<.input>` is imported and using it will save steps and prevent errors
- If you override the default input classes (`<.input class="myclass px-2 py-1 rounded-lg">)`) class with your own values, no default classes are inherited, so your
custom classes must fully style the input
This section intentionally minimal - core Phoenix concepts are covered in the main usage-rules sections below.
41 changes: 41 additions & 0 deletions installer/test/phx_new_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,40 @@ defmodule Mix.Tasks.Phx.NewTest do
assert_file("phx_blog/config/test.exs", fn file ->
refute file =~ ~s|config :phoenix_live_view|
end)

assert_file("phx_blog/AGENTS.md", fn file ->
refute file =~ "<.form"
refute file =~ "<.input"
refute file =~ "LiveView"
end)
end)
end

test "new with --no-live" do
in_tmp("new with no_live", fn ->
Mix.Tasks.Phx.New.run([@app_name, "--no-live"])

assert_file("phx_blog/AGENTS.md", fn file ->
refute file =~ "## Phoenix LiveView guidelines"
refute file =~ "LiveView streams"
refute file =~ "push_event"
refute file =~ "handle_event"
refute file =~ "phx-hook"
end)
end)
end

test "new with --no-ecto --no-live" do
in_tmp("new with no_ecto and no_live", fn ->
Mix.Tasks.Phx.New.run([@app_name, "--no-ecto", "--no-live"])

assert_file("phx_blog/AGENTS.md", fn file ->
refute file =~ "Ecto"
refute file =~ "changeset"
refute file =~ "## Phoenix LiveView guidelines"
refute file =~ "LiveView streams"
refute file =~ "phx-hook"
end)
end)
end

Expand Down Expand Up @@ -555,6 +589,13 @@ defmodule Mix.Tasks.Phx.NewTest do
assert file =~ "inputs: [\"*.{heex,ex,exs}\", \"{config,lib,test}/**/*.{heex,ex,exs}\"]"
refute file =~ "subdirectories:"
end)

assert_file("phx_blog/AGENTS.md", fn file ->
refute file =~ "Ecto"
refute file =~ "changeset"
refute file =~ "Ecto.Schema"
refute file =~ "Ecto.Changeset"
end)
end)
end

Expand Down
45 changes: 45 additions & 0 deletions usage-rules/ecto-forms.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
## Ecto Forms

### Creating forms from changesets

When using changesets, the underlying data, form params, and errors are retrieved from it. The `:as` option is automatically computed too. E.g. if you have a user schema:

defmodule MyApp.Users.User do
use Ecto.Schema
...
end

And then you create a changeset that you pass to `to_form`:

%MyApp.Users.User{}
|> Ecto.Changeset.change()
|> to_form()

Once the form is submitted, the params will be available under `%{"user" => user_params}`.

In the template, the form assign can be passed to the `<.form>` function component:

<.form for={@form} id="todo-form">
<.input field={@form[:field]} type="text" />
</.form>

Always give the form an explicit, unique DOM ID, like `id="todo-form"`.

### Avoiding form errors with changesets

**Always** use a form assigned via `to_form/1` or `to_form/2`, and the `<.input>` component in the template. In the template **always access forms this way**:

<%!-- ALWAYS do this (valid) --%>
<.form for={@form} id="my-form">
<.input field={@form[:field]} type="text" />
</.form>

And **never** do this:

<%!-- NEVER do this (invalid) --%>
<.form for={@changeset} id="my-form">
<.input field={@changeset[:field]} type="text" />
</.form>

- You are FORBIDDEN from accessing the changeset in the template as it will cause errors
- **Never** use `<.form let={f} ...>` in the template, instead **always use `<.form for={@form} ...>`**, then drive all form references from the form assign as in `@form[:field]`
45 changes: 45 additions & 0 deletions usage-rules/ecto-live-forms.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
## Ecto LiveView Forms

### Creating LiveView forms from changesets

When using changesets with LiveView, the underlying data, form params, and errors are retrieved from it. The `:as` option is automatically computed too. E.g. if you have a user schema:

defmodule MyApp.Users.User do
use Ecto.Schema
...
end

And then you create a changeset that you pass to `to_form`:

%MyApp.Users.User{}
|> Ecto.Changeset.change()
|> to_form()

Once the form is submitted, the params will be available under `%{"user" => user_params}`.

In the template, the form assign can be passed to the `<.form>` function component:

<.form for={@form} id="todo-form" phx-change="validate" phx-submit="save">
<.input field={@form[:field]} type="text" />
</.form>

Always give the form an explicit, unique DOM ID, like `id="todo-form"`.

### Avoiding form errors with changesets

**Always** use a form assigned via `to_form/1` or `to_form/2` in the LiveView, and the `<.input>` component in the template. In the template **always access forms this way**:

<%!-- ALWAYS do this (valid) --%>
<.form for={@form} id="my-form">
<.input field={@form[:field]} type="text" />
</.form>

And **never** do this:

<%!-- NEVER do this (invalid) --%>
<.form for={@changeset} id="my-form">
<.input field={@changeset[:field]} type="text" />
</.form>

- You are FORBIDDEN from accessing the changeset in the template as it will cause errors
- **Never** use `<.form let={f} ...>` in the template, instead **always use `<.form for={@form} ...>`**, then drive all form references from the form assign as in `@form[:field]`
2 changes: 1 addition & 1 deletion usage-rules/ecto.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
- Remember `import Ecto.Query` and other supporting modules when you write `seeds.exs`
- `Ecto.Schema` fields always use the `:string` type, even for `:text`, columns, ie: `field :name, :string`
- `Ecto.Changeset.validate_number/2` **DOES NOT SUPPORT the `:allow_nil` option**. By default, Ecto validations only run if a change for the given field exists and the change value is not nil, so such as option is never needed
- You **must** use `Ecto.Changeset.get_field(changeset, :field)` to access changeset fields
- **Never** use map access syntax (`changeset[:field]`) on changesets. You **must** use `Ecto.Changeset.get_field(changeset, :field)` to access changeset fields
- Fields which are set programatically, such as `user_id`, must not be listed in `cast` calls or similar for security purposes. Instead they must be explicitly set when creating the struct
- **Always** invoke `mix ecto.gen.migration migration_name_using_underscores` when generating migration files, so the correct timestamp and conventions are applied
12 changes: 6 additions & 6 deletions usage-rules/elixir.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,18 @@
you *must* bind the result of the expression to a variable if you want to use it and you CANNOT rebind the result inside the expression, ie:

# INVALID: we are rebinding inside the `if` and the result never gets assigned
if connected?(socket) do
socket = assign(socket, :val, val)
if some_condition?(data) do
data = transform(data)
end

# VALID: we rebind the result of the `if` to a new variable
socket =
if connected?(socket) do
assign(socket, :val, val)
data =
if some_condition?(data) do
transform(data)
end

- **Never** nest multiple modules in the same file as it can cause cyclic dependencies and compilation errors
- **Never** use map access syntax (`changeset[:field]`) on structs as they do not implement the Access behaviour by default. For regular structs, you **must** access the fields directly, such as `my_struct.field` or use higher level APIs that are available on the struct if they exist, `Ecto.Changeset.get_field/2` for changesets
- **Never** use map access syntax (`my_struct[:field]`) on structs as they do not implement the Access behaviour by default. For structs, you **must** access the fields directly, such as `my_struct.field` or use higher level APIs that are available on the struct if they exist
- Elixir's standard library has everything necessary for date and time manipulation. Familiarize yourself with the common `Time`, `Date`, `DateTime`, and `Calendar` interfaces by accessing their documentation as necessary. **Never** install additional dependencies unless asked or for date/time parsing (which you can use the `date_time_parser` package)
- Don't use `String.to_atom/1` on user input (memory leak risk)
- Predicate function names should not start with `is_` and should end in a question mark. Names like `is_thing` should be reserved for guards
Expand Down
4 changes: 2 additions & 2 deletions usage-rules/html.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

- Phoenix templates **always** use `~H` or .html.heex files (known as HEEx), **never** use `~E`
- **Always** use the imported `Phoenix.Component.form/1` and `Phoenix.Component.inputs_for/1` function to build forms. **Never** use `Phoenix.HTML.form_for` or `Phoenix.HTML.inputs_for` as they are outdated
- When building forms **always** use the already imported `Phoenix.Component.to_form/2` (`assign(socket, form: to_form(...))` and `<.form for={@form} id="msg-form">`), then access those forms in the template via `@form[:field]`
- When building forms **always** use the already imported `Phoenix.Component.to_form/2` (`assign(:form, to_form(...))` and `<.form for={@form} id="msg-form">`), then access those forms in the template via `@form[:field]`
Copy link
Member

Choose a reason for hiding this comment

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

Phoenix.Component already assumes LiveView, so this change still assumes LiveView. I would keep it as is, I think the templates one below is fine.

- **Always** add unique DOM IDs to key elements (like forms, buttons, etc) when writing templates, these IDs can later be used in tests (`<.form for={@form} id="product-form">`)
- For "app wide" template imports, you can import/alias into the `my_app_web.ex`'s `html_helpers` block, so they will be available to all LiveViews, LiveComponent's, and all modules that do `use MyAppWeb, :html` (replace "my_app" by the actual app name)
- For "app wide" template imports, you can import/alias into the `my_app_web.ex`'s `html_helpers` block, so they will be available to all templates and all modules that do `use MyAppWeb, :html` (replace "my_app" by the actual app name)

- Elixir supports `if/else` but **does NOT support `if/else if` or `if/elsif`**. **Never use `else if` or `elseif` in Elixir**, **always** use `cond` or `case` for multiple conditionals.

Expand Down
38 changes: 1 addition & 37 deletions usage-rules/liveview.md
Original file line number Diff line number Diff line change
Expand Up @@ -186,46 +186,10 @@ You can also specify a name to nest the params:
{:noreply, assign(socket, form: to_form(user_params, as: :user))}
end

#### Creating a form from changesets

When using changesets, the underlying data, form params, and errors are retrieved from it. The `:as` option is automatically computed too. E.g. if you have a user schema:

defmodule MyApp.Users.User do
use Ecto.Schema
...
end

And then you create a changeset that you pass to `to_form`:

%MyApp.Users.User{}
|> Ecto.Changeset.change()
|> to_form()

Once the form is submitted, the params will be available under `%{"user" => user_params}`.

In the template, the form form assign can be passed to the `<.form>` function component:
In the template, the form assign can be passed to the `<.form>` function component:

<.form for={@form} id="todo-form" phx-change="validate" phx-submit="save">
<.input field={@form[:field]} type="text" />
</.form>

Always give the form an explicit, unique DOM ID, like `id="todo-form"`.

#### Avoiding form errors

**Always** use a form assigned via `to_form/2` in the LiveView, and the `<.input>` component in the template. In the template **always access forms this**:

<%!-- ALWAYS do this (valid) --%>
<.form for={@form} id="my-form">
<.input field={@form[:field]} type="text" />
</.form>

And **never** do this:

<%!-- NEVER do this (invalid) --%>
<.form for={@changeset} id="my-form">
<.input field={@changeset[:field]} type="text" />
</.form>

- You are FORBIDDEN from accessing the changeset in the template as it will cause errors
- **Never** use `<.form let={f} ...>` in the template, instead **always use `<.form for={@form} ...>`**, then drive all form references from the form assign as in `@form[:field]`. The UI should **always** be driven by a `to_form/2` assigned in the LiveView module that is derived from a changeset