Delegate to Roar's represent w/o options argument

[jcwilk]

Small change that seems to grant us all of the niceties of Roar's represent for free.

The main thing I was running into that Grape::Roar does not support but that Roar does support is doing things like...

JSONDecoratorClass.represent([obj1,obj2]).to_json

Since Grape::Roar never calls https://github.com/apotonick/representable/blob/master/lib/representable/represent.rb#L3 so the Representable::ForCollection stuff isn't utilized, along presumably with any other features included in https://github.com/apotonick/representable/blob/master/lib/representable.rb#L22-L27 but they seem less exciting than being able to pass in an array of objects.

Only other way I can seem to figure to be able to represent an array of objects is to make a whole separate representer class just for a plural version of a resource, which seems really verbose.

Specs still pass fine. Let me know if you want me to add a spec for the array stuff, though I guess I see this as generally tapping into Representable in a more complete way... Which you can't really write a test for, but if you think testing for enabling arrays specifically is valuable I can throw one in, lmk.

[jcwilk]

Not sure what's up with that failure, seems intermittent maybe? Looks like it's having trouble downloading some ruby version.

[dblock]

I like this change. It needs documentation, tests, and a CHANGELOG entry.

You should fix the build too, use rbx-2 in .travis.yml and add it to ignore_failures.

[jcwilk]

@dblock - Ok, I added specs to cover the array usage.

Before I make an entry in the CHANGELOG and README.md though, there's a problem though that I need advice on... An example in README.md no longer works, specifically:

module ProductsRepresenter
  include Roar::JSON::HAL
  include Roar::Hypermedia
  include Grape::Roar::Representer

  collection :entries, extend: ProductRepresenter, as: :products, embedded: true
end

get 'products' do
  present Product.all, with: ProductsRepresenter
end

The problem is that it now taps into Representer's collection behavior and feeds the individual entries into the representer automatically, so it's no longer possible to write a representer for an array of objects.

However, aside from it being breaking behavior, I think this is a good change. Apologies if I fumble on the terminology a bit here, but an array of resources is not a resource and does not deserve the ability to define its own rels. An array is essentially a primitive type and has no meaning except as a property of some other resource. To be clear, this brings Grape::Roar closer in-line with Roar... It's not possible to use an array as a resource in Roar AFAIK, and nor is it in Representable.

I think the only way you could do it is if you extended the array itself with the representer and then presented it without the with: argument, but that seems pretty hacky... I suppose it could be recommended in notes on upgrading though for folks who didn't want to rewrite it in a more purist manner?

Note that tests around Order still work (nested_representer_spec.rb), using an array within a resource still works the same as it always did.

If it wasn't described in README.md already then I'd categorize it as a fixed bug that people may be improperly depending on in their code, and is thus still a breaking change, but for the better since they should represent those arrays as proper resources with a class rather than just a floating array.

I'm very interested to hear your take on it though. Sorry that this is turning out to be a bother.

[dblock]

Hmmmm, this is kinda a big deal. I've seen this kind of usage in many places, I definitely use it all over the place, like here.

I guess the question is, how does one present a collection of objects in the new world?

[jcwilk]

Well, you can still present an array of resources... It's actually exactly the same you just can't write a representer for the array itself, so if you want to make a self rel to the array you cannot. Luckily, README.md did not do this in the example, so the above quoted example would be accomplished I think identically by simply:

get 'products' do
  present Product.all, with: ProductRepresenter #NB Product not Products
end

It will then render that as a JSON array of resources (as described in my spec additions).

[jcwilk]

So in a nutshell, it's an easy fix if you're using barebone collection representers for your barebone arrays... Just delete the representer and reference the singular resource representer instead. Requires code change but results in less code.

The tricky part is if people are treating barebone arrays as formalized rels, but they're hypermedia-ing wrong in that case.

Also for your linked example it's the former, you'd just need to delete that TeamsPresenter module and change that line to TeamPresenter.

[dblock]

I don't think we want an array of hypermedia representations. A lot of times you want other fields in there, such as total_count, for example. How would I do that? The response is also not a valid HAL document I think in that case.

I don't want to make a change like this irresponsibly, I'd like to leave this open and see if other people comment. I'll do a code review of course either way. You might want to look for a way to keep the old functionality though, basically have it both ways.

[dblock]

Along with README and CHANGELOG make sure to write an UPGRADING with a section about what's affected.

[dblock]

Expand the string into a Hash and compare JSON.parse to that, so it's more readable, multiple lines, etc.

[dblock]

"but they're hypermedia-ing wrong in that case." - I personally feel the opposite way, I wonder what a library like Hyperclient "thinks" when it sees a plain array of hypermedia responses? Maybe this can/should/has been brought up on the HAL mailing list?

[jcwilk]

Ok, is it alright if I wait to change CHANGELOG, README.md, and the UPGRADING section until the changes are approved?

I'll try to think of a way to maintain previous behavior... But I don't know if it's possible without forcing them to be explicit about automatically turning each array element into the specified representer, which kind of defeats the purpose of the PR since I was hoping it would bring Grape::Roar and Roar closer together rather than further apart.

[dblock]

I hear you @jcwilk, I am not saying this is a bad idea, I just see unintended (or intended) consequences that aren't going to make people happy.

[dblock]

Also maybe write a failing spec here for the array representers scenario, I'll try to help fix.

[jcwilk]

"I feel the opposite way, I wonder what a library like Hyperclient "thinks" when it sees a plain array of hypermedia responses?" - Well the thing is that there's two scenarios:

1 - It's an arbitrary array, has no inherent meaning, is not a relation from anything else, and is just a junk pile of elements which each are individually formalized with their own self rels but there's no coherent way to refer to collection of all of them in relation to anything else.

2 - It's a meaningful collection which represents a relation from something else, like all products ordered by a user, or all line items of an order, or something like that, and it deserves its own properties, a rel, and maybe even a self rel.

For 1, IMO it should really simply be an array of hypermedia resources. IIRC (it's been awhile) clients vary on how they handle this, the more flexible ones go "hey, this is an array of resources... I know how to handle resources, and arrays are simple." You could maybe get purist about it and try to think of some overarching meaning of all existing products to turn it into a resource, but I'm a little out of my element here about what is and isn't correct and it gets a little philosophical.

For 2, it's more straightforward and is just a normal hypermedia rel.

[jcwilk]

Sure, I can write a failing spec.

[jcwilk]

@dblock - failing spec here

[dblock]

Thanks, I'll try to spend some time on this.