Add support for dynamic string-links in JsonApi adapter. #1406
Conversation
| @@ -13,6 +13,10 @@ class LinkAuthorSerializer < ActiveModel::Serializer | |||
| end | |||
|
|
|||
| link :other, '//example.com/resource' | |||
There was a problem hiding this comment.
Off topic: given that JSON API is undecided on relative links, I'd rather not fill our examples with them
There was a problem hiding this comment.
Agreed, however this is not a relative link. //resource would be undefined, but //example.com/resource is a valid url.
There was a problem hiding this comment.
@beauby words... words.. I meant 'protocol relative'. Sorry I typo'ed again :(
There was a problem hiding this comment.
I don't feel strongly about this, although it is not clear from the discussion whether they are disallowing relative urls or protocol-relative urls.
There was a problem hiding this comment.
Yeah, I just updated my question to clarify that
| end | ||
|
|
||
| def serializable_hash |
There was a problem hiding this comment.
I'm not sure if serializable_hash is a good name considering it can be a string. What's your point on this?
There was a problem hiding this comment.
Indeed, it is not a perfect name. The rationale was that serializable_hash is already used all around in AMS for objects that are "ready to be transformed into JSON", i.e. no further treatment is required (used in SerializableResource.new(resource).serializable_hash, PaginationLink.new(stuff).serializable_hash).
There was a problem hiding this comment.
@bf4 I'm not satisfied with value but you're right, it's probably better to keep it consistent for now and possibly update everything in a subsequent commit.
There was a problem hiding this comment.
I think it mainly looks weird because we're passing value into. Maybe just have as_json and to_json?
There was a problem hiding this comment.
as_json is the way to go IMO (for the record, as_json means build something ready to be directly transformed into JSON, i.e. a Hash mainly, but we could extend the meaning to be "something that will eventually be part of a Hash representing a JSON document", and to_json means actually building a JSON string from the object).
| if value.respond_to?(:call) | ||
| self._value = instance_eval(&value) | ||
| else | ||
| self._value = value |
There was a problem hiding this comment.
What's our desired behavior if someone does
link :foo do
meta 'thing'
'https://example.com/doohickie'
end?
right now it would only return the block's return value
There was a problem hiding this comment.
We should also probably comment that in the block form, if meta and/or href are used, value is nil and the Link is an object. If neither are used, the Link is a URI (the return value of the block). If both are used and the block has a return value....?
There was a problem hiding this comment.
The way I see it, providing a return value to the block should be interpreted as "I'm a big boy and this is my link value". Otherwise, "building" a link object through the DSL should be done, well, via the DSL. What are your thoughts?
There was a problem hiding this comment.
Agreed that it should be stated clearly.
There was a problem hiding this comment.
So, let's update and warn against the scenario in https://github.com/rails-api/active_model_serializers/pull/1406/files#r48668245 and ok to merge
There was a problem hiding this comment.
with the serializable_hash to as_json change in https://github.com/rails-api/active_model_serializers/pull/1406/files#r48702394
|
All concerns taken care of. I'll merge later today unless someone raises other concerns. |
| attr_reader :object, :scope | ||
|
|
||
| private | ||
|
|
||
| def to_hash |
There was a problem hiding this comment.
block_value || hash_value ? to_hash doesn't really jive well with how I think of the method
There was a problem hiding this comment.
granted, this is internal api, so just talking about naming
|
@beauby Please add changelog and docs, and it's good to go. I don't really like the internals of the |
|
|
||
| # Use the return value of the block unless it is nil. | ||
| if value.respond_to?(:call) | ||
| self._value = instance_eval(&value) |
There was a problem hiding this comment.
Could you please explain why you used an attr_accessor for _value instead of using a reader and calling @_value = instance_eval(&value)? I don't see why _value is different than object or scope.
There was a problem hiding this comment.
You're right, there's no reason. There just seemed to be a trend (which I kinda disapprove actually) in AMS to move towards accessors rather than ivars. I'll update it.
There was a problem hiding this comment.
agreed it should be a naked instance variable here
my general thinking: setters and getters are, all things being equal, a better way to handle accessing instance values, since it protects the code against changes and typos. However, there's no need to message dispatch when setting the value in an initializer, so a reader-only is fine there. As to eschewing accessors all together, I'd probably only do that for performance reasons, or when maybe when I'm certain there code won't change often so mis-typing a variable name is unlikely.
|
@beauby squash commits, add changelog and good to merge. I want to release rc4 soon so am trying to get whatever prs with good features that can be merged, in. |
beauby
force-pushed
the
dynamic-jsonapi-string-links
branch
3 times, most recently
from
cb35d6d to
38b71ea
Compare
|
PR ready, waiting for Travis. |
beauby
force-pushed
the
dynamic-jsonapi-string-links
branch
from
38b71ea to
30d8414
Compare
Add support for custom dynamic valued links in JsonApi adapter.
Solves #1400.