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

Optional plug dependency #167

Open
lasseebert opened this issue Jan 28, 2019 · 17 comments
Open

Optional plug dependency #167

lasseebert opened this issue Jan 28, 2019 · 17 comments
Labels

Comments

@lasseebert
Copy link

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
Copy link

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

@lasseebert
Copy link
Author

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

@doomspork
Copy link
Member

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

@lasseebert
Copy link
Author

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

@CrowdHailer
Copy link

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

@doomspork
Copy link
Member

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
Copy link

CrowdHailer commented Feb 3, 2019

@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
Copy link
Member

@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
Copy link
Author

lasseebert commented Feb 27, 2019

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:

  • All the plugs. These could just be Plug-specific wrappers around common functionality for e.g. extracting the JSON:API query.
  • The Serializer, which could also rely on a more general non-plug-specific serializer
  • The View, called from the Serializer with a conn.

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
Copy link
Author

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
Copy link
Contributor

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:

  • Resource. These would be the things that describe the attributes, relationships, and links a JSON:API resource has. Double points if we can figure out how to automagically generate JSON Schemas from these. This layer should be the same regardless if you're targetting Raxx or Plug.
  • Serializer. I think this would largely be unchanged from what it is, sans any shims needed for either Plug or Raxx.
  • View. I'm not familiar with Raxx, but in the Plug world this would be the Views we know and love. They'd take a resource, and the Serializer, and bridge the two.

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
Copy link
Author

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

@doomspork doomspork added the rfc label Feb 28, 2019
@snewcomer
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

snewcomer commented Mar 2, 2019

@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
Copy link
Author

@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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants