Remove ja_resource #864
Comments
|
To add, we can do this gradually. Switch one or two major controllers to it as examples, then volunteers can take it over bit by bit. Once everything is switched, we remove the package from our dependencies. |
|
I'd be interested in helping out here. |
|
@snewcomer This is still at a "suggestion" level, but if we go through with it, your help is much appreciated. |
|
I agree with this move and think it actually decreases the potential overhead for contributors, which is always a plus in my book. |
|
Can we make some smaller issues to go after this, and @snewcomer would you be interested now in tackling some of those? |
|
@joshsmith yep I am available to help with this. |
|
@begedin if you have some time to break this down the way you'd like to see it, let me know. |
|
@joshsmith I'd have to figure out how to do it first, using an example of a single controller. There's a lot of coupling going on under the hood at the moment, with our authorization being implemented around ja_resource. I'd have to take a look and try at least a small example first. I'll see what I can do. |
|
@joshsmith It was difficult to grasp by just looking, so I went ahead and submitted a PR using the example of the comment controller - #867 Some notes on it
|
|
#867 deals with switching one controller away. Using the knowledge gained from there, here's the full procedure on how to switch Procedure
That should be the whole procedure. We already have controller tests in place, so if the process has been done correctly, the tests should pass. Some additional steps might be necessary, however, if there are some specifics to an action. For those, we will be here to provide feedback. How to rewrite controller actions@spec index(Conn.t, map) :: Conn.t
def index(%Conn{} = conn, %{} = params) do
with resources <- ResourceModule |> Query.id_filter(params) |> Repo.all do
conn |> render("index.json-api", data: resources)
end
end
@spec show(Conn.t, map) :: Conn.t
def show(%Conn{} = conn, %{"id" => id}) do
with %ResourceModule{} = resource <- ResourceModule |> Repo.get(id) do
conn |> render("show.json-api", data: resource)
end
end
@spec create(Plug.Conn.t, map) :: Conn.t
def create(%Conn{} = conn, %{} = params) do
with %User{} = current_user <- conn |> Guardian.Plug.current_resource,
{:ok, :authorized} <- current_user |> Policy.authorize(:create, params),
{:ok, %Comment{} = resource} <- %ResourceModule{} |> ResourceModule.create_changeset(params) |> Repo.insert do
conn |> put_status(:created) |> render("show.json-api", data: resource)
end
end
@spec update(Conn.t, map) :: Conn.t
def update(%Conn{} = conn, %{"id" => id} = params) do
with %ResourceModule{} = resource <- ResourceModule |> Repo.get(id),
%User{} = current_user <- conn |> Guardian.Plug.current_resource,
{:ok, :authorized} <- current_user |> Policy.authorize(:update, resource),
{:ok, %ResourceModule{} = resource} <- resource |> ResourceModule.changeset(params) |> Repo.update do
conn |> render("show.json-api", data: resource)
end
endSo to recap, the actions work like
|
ToDo ListPhase 1
Phase 2
|
joshsmith
added
Difficulty: Easy
Priority: High
Size: XL
Skill: Elixir
Skill: Phoenix
and removed
needs clarification
labels
|
@snewcomer thanks to @begedin there are some real specifics here for how to tackle working on this. |
|
Awesome! Will be tacking a few tomorrow with a coworker here! |
|
If available, id like to contribute on 1 or 2 controllers myself. Perhaps |
|
@vishaldeepak, @snewcomer Feel free to do so. Also feel free to create individual issues for those parts you would like to tackle. As long as the body of the issue contains |
joshsmith
modified the milestones:
Move away from ja_resource and canary in controllers,
Remove ja_resource
|
I've moved the remaining parts of phase 2 tasks into #1069 |
Problem
ja_resourcehas been giving us trouble before and will likely continue to give us more trouble.Our initial reasoning for introducing it to our project was to reduce our controller boiler plate. However, with the way our other tools developed, this benefit has been gradually reducing and we now barely have any less boilerplate than before, and the cost has been reduced readability of code, as well as performance, in some cases.
ja_resourcewas the main blocker in doing so.ja_resourceCodeCorpsWebnamespace, while models are inCodeCorps, so each controller needs to override theja_resourcemodel/0function to get it workingja_resourcedepends heavily on the model concept to generally workFallbackControllerand action fallbacks, we can reduce boilerplate that wayConclusion
With all that in mind, my recommendation is to get rid of
ja_resourcecompletely. This would involve...Requirements
FallbackControllerto reduce the boilerplate that wayNotes
The advantages are
On a personal note, it's my opinion that we introduced
ja_resourceearly in our elixir experience when we still tried to make it work sort of like ruby. Now that I have more experience with the language and the phoenix framework, the negatives, to me, seem to outweigh the positives.With a simple REST api, it might still work, but with the things we do here, I don't think it does.
The text was updated successfully, but these errors were encountered: