beam-community / jsonapi

JSON:API Serializer and Query Handler for Elixir
https://hex.pm/packages/jsonapi
MIT License
497 stars 78 forks source link

Optional plug dependency #167

Closed lasseebert closed 4 months ago

lasseebert commented 5 years ago

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:

  1. The existing public API remains untouched
  2. Plug will become an optional dependency
  3. The package is usable without plug
jeregrine commented 5 years ago

@lasseebert PR's always welcome if you'd like to give it a shot!

lasseebert commented 5 years ago

@jeregrine Super. It will probably take a little while, since my calendar is a bit full, but I intend to pull it off :)

doomspork commented 5 years ago

Hello hello @lasseebert! Happy to lend a hand if you need it. Feel free to ping me here, Twitter, or Slack 😁

lasseebert commented 5 years ago

@doomspork, thanks ☺️ I'll let you know when I begin on something.

CrowdHailer commented 5 years ago

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. :thinking:

doomspork commented 5 years ago

I'm leery of suggesting it but do we want to consider jsonapi_plug and jsonapi_raxx packages, with the former being included by default maybe?

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.

CrowdHailer commented 5 years ago

@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. CORS module which was called from CORS.Plug and CORS.Raxx and so there was just a single check for loaded modules. i.e. if plug loaded define the whole CORS.Plug module. I've seen several libraries handle optional dependencies this way.

Maybe this is best moved to a forum question, how best to handle optional dependencies.

doomspork commented 5 years ago

@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.

lasseebert commented 5 years ago

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.

lasseebert commented 5 years ago

So that we have e.g. JSONAPI.Core.Serializer and JSONAPI.Core.View, which have the needed functionality, but is not necessarily convinient to use as a standalone. But with the Core functionality anyone could make a jsonapi_(raxx | phoenix | whatever) wrapper.

jherdman commented 5 years ago

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.

lasseebert commented 5 years ago

@jherdman I really like the split of Resource and View, so that Resource is independent of any framework or HTTP library. Thanks for the insights!

snewcomer commented 5 years ago

@lasseebert Just curious, how do you plan on using jsonapi with Raxx? raxx_view? Similar to use JSONAPI.View in a view.ex file? I have a few thoughts on how this could be implemented (some simple), but would be helpful to understand the use case!

lasseebert commented 5 years ago

@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.

snewcomer commented 5 years ago

@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 use JSONAPI.View, adapter: JSONAPI.Raxx and follow the same implementation as we currently have, it should be very easy to optionally include Plug or not based on the adapter. However, if not a similar implementation (e.g. you only need to say build part of a jsonapi document), then might require a larger refactor to pull pieces out. Also, what about processing a JSONAPI request? Any initial thoughts or feelings?

lasseebert commented 5 years ago

@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.

snewcomer commented 5 years ago

@lasseebert Totally understandable! Working on some non breaking ideas this week then.

github-actions[bot] commented 4 months ago

This issue has been automatically marked as "stale:discard". We are sorry that we haven't been able to prioritize it yet. If this issue still relevant, please leave any comment if you have any new additional information that helps to solve this issue. We encourage you to create a pull request, if you can. We are happy to help you with that.

github-actions[bot] commented 4 months ago

Closing this issue after a prolonged period of inactivity. If this issue is still relevant, feel free to re-open the issue. Thank you!