-
Notifications
You must be signed in to change notification settings - Fork 80
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
Optional plug dependency #167
Comments
@lasseebert PR's always welcome if you'd like to give it a shot! |
@jeregrine Super. It will probably take a little while, since my calendar is a bit full, but I intend to pull it off :) |
Hello hello @lasseebert! Happy to lend a hand if you need it. Feel free to ping me here, Twitter, or Slack 😁 |
@doomspork, thanks |
I am also happy to help out, I had a quick look through the code and it looks like there are a lot of places that rely on a plug and in several cases it's just to generate urls so it would be better just to pass the root url/host as a string or maybe URI struct. Making that change might be a good start. 🤔 |
I'm leery of suggesting it but do we want to consider When we looked to remove our dependency on Plug in Guardian it resulted in a lot of awkward checks for loaded modules; it really has a way to messing up the codebase. |
@doomspork Might be a bit off topic, but do you have a PR that shows the changes you had to make to guardian? I've started on a CORS library and basic auth library where there was an amount of shared code e.g. Maybe this is best moved to a forum question, how best to handle optional dependencies. |
@CrowdHailer I don't have a PR but that's how we do it: https://github.com/ueberauth/guardian/blob/master/lib/guardian/plug/ensure_authenticated.ex#L1 if Code.ensure_loaded?(Plug) do
defmodule Guardian.Plug.EnsureAuthenticated do
end
end It works. It's not pretty. Especially when you start adding in other optional packages. |
I have had a busy time at work with things unrelated to this, which is why I haven't really started on it yet. I now had a more thorough view of the code and it seems to me that the places that needs attention if we're going to make Plug optional is:
Perhaps the best approach is to extract common functionality into a certain namespace, e.g. JSONAPI.Core, which is meant to be used as building blocks for more specific implementations using e.g. Plug, Raxx or whatever. And then of course still support Plug as the default in this library. |
So that we have e.g. |
I'm pretty excited about the refactoring that'll come out of this, and I think we can do it a bit ahead of time too. One of the things that bugs me about our View module is the conflation of the specification of a JSON:API resource, and how it fits into the Plug pipeline (i.e. this stuff, and also this). It'd be neat to see this split up a bit as a result of what's going on here into a few different and distinct ideas:
The advantage of our current approach is that conflating the Specification and View means we don't need any sort of boilerplate — you define a View, and then rest is pretty much golden. I think the future could look something like this: # Assume a Phoenix app, and we want some of the current JSONAPI.View magic...
# lib/my_app_web/views/car_view.ex
defmodule MyAppWeb.CarView do
use JSONAPI.Resource.Plug
def attributes do
[:brand, :colour]
end
end And if we wanted a sharp distinction... # lib/my_app_web/resource/car_resource.ex
defmodule MyAppWeb.CarResource do
use JSONAPI.Resource
def attributes do
[:brand, :colour]
end
end
# lib/my_app_web/views/car_view.ex
defmodule MyAppWeb.CarView do
use JSONAPI.View.Plug
end Obviously I'm spitballing, and I need to do some homework on Raxx. |
@jherdman I really like the split of Resource and View, so that Resource is independent of any framework or HTTP library. |
@lasseebert Just curious, how do you plan on using jsonapi with Raxx? raxx_view? Similar to |
@snewcomer I did not yet plan an actual integration with Raxx. My first thought was to handroll something that fit my application with building blocks from some JSON:API library. It turned out I couldn't find such a library that did not require Plug. So for starters, my intention is to make the central building blocks available for non-plug apps. Next step could be to make a Raxx-specific integration. |
@lasseebert So sounds like you don't need to build a full jsonapi document and instead just parts of it?? Essentially, if you just need something like |
@snewcomer: My initial use-case was to render a JSON:API document (attributes and relationships), so the serializer was my main concern. However, since this issue was created, my use-case (from my daily work) has changed from a JSON:API Raxx application to a more simple proxy calling an existing plug-based JSON:API application. This means I can not throw as much time at it as I would like to. If someone else starts a refactor, I will gladly help with reviews and implementation of some parts. |
@lasseebert Totally understandable! Working on some non breaking ideas this week then. |
I am using Raxx (and not Plug) in a project and would like to use JSONAPI with that.
I have played a little with the code, and it seems that with a bit of refactoring, the plug dependency of JSONAPI could be optional and all the "core" json:api stuff be publicly accessible API without needing to have a Plug.Conn.
@jeregrine, if you are interested, I would like to give it a shot, and see if I can make that refactor in a way so that:
The text was updated successfully, but these errors were encountered: