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

Include data key when lazy-loaded relationships are included #10

Conversation

dpikt
Copy link
Contributor

@dpikt dpikt commented Oct 4, 2019

Resolves Netflix/fast_jsonapi#357

When a relationship is listed under include:, it will add the data key to the record hash, allowing the included relationships to be reconstructed.

end
end

def meta_hash(record, params = {})
meta_to_serialize.call(record, params)
end

def record_hash(record, fieldset, params = {})
def record_hash(record, fieldset, includes_list, params = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If includes_list doesn't have a default value here, it could be a breaking change.

@@ -34,12 +34,12 @@ def initialize(
@lazy_load_data = lazy_load_data
end

def serialize(record, serialization_params, output_hash)
def serialize(record, included, serialization_params, output_hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a breaking change. New parameters should generally be appended to the list and included a default.

@jopotts
Copy link
Contributor

jopotts commented Oct 7, 2019

@kpheasey I've looked into the possible breaking change issues with the method params on record_hash, relationships_hash and Relationship#serialize, and none seem to be part of the standard public interface. It is possible that a deeply coupled use of the gem might be affected, but I think the fix this change is addressing is worth it. The alternative would be to make the new params non-breaking, but it would make these internal methods ugly. Tricky.

@dpikt Can you look into the conflict in the spec. Thanks.

@kpheasey kpheasey merged commit 8e23831 into jsonapi-serializer:dev Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants