-
Notifications
You must be signed in to change notification settings - Fork 1
Adding ability to render relationships only #5
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
base: main
Are you sure you want to change the base?
Adding ability to render relationships only #5
Conversation
mjeffrey18
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Apologies for the delay.
Everything looks great, just a few DSL comments.
Plus, before we merge, let's bump the minor version and update the docs?
| def initialize(@name = nil, @children = nil) | ||
| end | ||
|
|
||
| def embed(@embed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use something like def include?(@include)? Feels more implicit for the desired outcome.
| self | ||
| end | ||
|
|
||
| def have_relation?(name : Symbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping consistency with the rest of the codebase
| def have_relation?(name : Symbol) | |
| def has_relation?(name : Symbol) |
| end | ||
|
|
||
| def traverse(path) | ||
| children.not_nil!.find { |child| child.name == path }.not_nil! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw an error here with a message to inform the user if they don't have child nodes?
| children.nil? || children.try &.empty? | ||
| end | ||
|
|
||
| def traverse(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to find a better API for the end user. Thoughts?
| def traverse(path) | |
| def relationship(path) # OR find_relationship(path) |
|
Hey @AskarZinurov, thanks again for your effort on this. I tried running the benchmark script and noticed the speed has dropped a bit for both benchmarks MainFeatureI tried to change the new classes to structs, which helped gain some speed, but the specs are failing. We should keep the general execution of the shard to gain this feature, although I understand there might be minor speed drops as features are added. Another option could be to change the API. Looking at the Ruby gem, they only use included for has_many relationships and explicitly allow included - https://github.com/jsonapi-serializer/jsonapi-serializer#compound-document Maybe that could be our approach, but using the existing setup for speed. We could end up with something like this class UserSerializer < FastJSONAPISerializer::Base(User)
belongs_to :post_code, PostCodeSerializer
belongs_to :account, serializer: AccountSerializer, key: "user_account"
belongs_to :organisation, serializer: OrganisationSerializer, optional: true
endThen we can assume the user serializer will always have the Like so: UserSerializer.new(resource).serialize{
"data": {
"id": "1",
"type": "user",
"attributes": {
"name": "Me"
},
"relationships": {
"post_code": {
"data": {
"id": "101",
"type": "post_code"
}
},
"account": {
"data": {
"id": "101",
"type": "account"
}
}
}
}
}UserSerializer.new(resource).serialize(
includes: { :organisation => [:organisation] }
){
"data": {
"id": "1",
"type": "user",
"attributes": {
"name": "Me"
},
"relationships": {
"post_code": {
"data": {
"id": "101",
"type": "post_code"
}
},
"account": {
"data": {
"id": "101",
"type": "account"
}
},
"organisation": {
"data": {
"id": "1323",
"type": "organisation"
}
}
}
}
}The best way to accomplish this would maybe be to add a new relationship set on the base class. private getter _relationships : Set(String)Then, populate this based on the I believe this should have a very minor performance hit Thoughts? |
mjeffrey18
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment on speed, I can help out with some of these changes also. Ping me when you have had a chance to go over everything.
Hi! First of all want to thank you for your work. It is really very pleasant, light and fast lib.
In this PR I am adding abilitity to render relations only, without requirement to include associated objects later into
includesarray.The relations can be rendered in mixed way, so some configured relations will be included, and some not. The default behaviour is not changed, examples of usage can be found in specs.
Will be glad to hear your feedback. Thank you!