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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow sending back relationship.data hints without includes #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acorncom
Copy link
Contributor

@acorncom acorncom commented Nov 1, 2018

Fixes #47

@tuyakhov this is not as pretty as I'd like, but was intended to continue our discussions around how to optionally allow use cases like #47 Happy to have you rip this specific implementation apart, as I have no love for the code 馃槈 but would like to discuss further thoughts on ways we can get the desired functionality in as I've hit another case where it would be very helpful to have this functionality.

This is a backwards compatible change that allows a consumer to expose data that the serializer can use through the extraFields method:

public function extraFields()
{
    return [
        'car', // documented approach
        'anotherNormalCar' => function($model) { // callback approach that works
            return $model->car;
        },

        // unlocked functionality
        'car' => function($model) {
            return [
                'resource' => $model->car,
                'data' => new ResourceModelIdentifier($model->car_id, 'cars'),
            ];
        },
    ];
}

In using this, exposing callback setups and hashes with names seems fairly fragile. We could potentially expose a different class that internally does something similar. Possibly something akin to this:

public function extraFields()
{
    return [
        'car' => function($model) {
            return new LinkableResourceModel($relationshipMethod, $localForeignKey, $japiType),
        },
    ];
}

Thoughts?

@acorncom
Copy link
Contributor Author

acorncom commented Feb 2, 2019

@tuyakhov any thoughts on this PR? It's been running in production for us without issue for a while now ...

@tuyakhov
Copy link
Owner

tuyakhov commented Feb 3, 2019

@acorncom I am sorry for taking so long to reply to your PR.
It looks good to me. I was trying to think of other ways to allow this functionality. But couldn't come up with anything better. So I am going to merge this PR and include the changes to the next release.
Would you be able to update the README file as well, so that others know that this functionality exists?
Thank you.

@acorncom
Copy link
Contributor Author

@tuyakhov apparently I missed your comment here somehow. Still think we want to merge this? If so, I'll update the README and get this ready for a merge

@tuyakhov
Copy link
Owner

@acorncom Yes, we can definitely merge this one.

@tuyakhov tuyakhov closed this Nov 21, 2019
@tuyakhov tuyakhov reopened this Nov 21, 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.

Allow addition of relationship information without full include data
2 participants