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/exclude does not work on the Resource class #868

Closed
Tofandel opened this issue Sep 19, 2024 · 4 comments
Closed

Include/exclude does not work on the Resource class #868

Tofandel opened this issue Sep 19, 2024 · 4 comments

Comments

@Tofandel
Copy link
Contributor

Tofandel commented Sep 19, 2024

✏️ Describe the bug
Using include('someProp') or exclude on a DataClass that extends the Resource class does not work because Resource is missing the ContextableDataContract but has the trait

This means that this condition

if ($this instanceof IncludeableDataContract && $this instanceof ContextableDataContract) {
$transformationContext->mergePartialsFromDataContext($this);
}

Cannot pass and the transformationContext does not contain any partials

Here is a pest case that demonstrates the issue

it('can work with lazy laravel data collections', function () {
    $dataClass = new class () extends Resource {
        #[DataCollectionOf(SimpleData::class)]
        public Lazy|Collection $lazyCollection;
    };

    $dataClass->lazyCollection = Lazy::create(fn () => collect([
        SimpleData::from('A'),
        SimpleData::from('B'),
    ]));

    expect($dataClass->toArray())->toMatchArray([]);

    expect($dataClass->include('lazyCollection')->toArray())->toMatchArray([
        'lazyCollection' => [
            ['string' => 'A'],
            ['string' => 'B'],
        ],
    ]);
});
Tofandel added a commit to Tofandel/laravel-data that referenced this issue Sep 19, 2024
@Tofandel Tofandel changed the title Include/exclude does not work on the Ressource class Include/exclude does not work on the Resource class Sep 19, 2024
@SethSharp
Copy link

I think adding ContextableData to the base Resource class might be opinionated to your use case. I ran into the same situation but came across this section of the docs - Traits and Interfaces. Which suggests that Resource is a generic implementation - this package is designed to allow us to add our own functionality where necessary.

@Tofandel
Copy link
Contributor Author

Tofandel commented Sep 23, 2024

I really don't think so because the trait to allow include and exclude Is used on it as well as the IncludeableData contract, if that was the case, the trait and interface should not be on it, and the doc suggests using it as an alternative to Data that doesn't have data validation (and not exclude include) and thus performs a bit better

These resource classes have as an advantage that they won't validate data or check authorization, They are only used to transform data which makes them a bit faster.

https://spatie.be/docs/laravel-data/v4/as-a-resource/from-data-to-resource

I don't see how wanting to use the lazy property feature of the library on a resource is opinionated

I think you are mixing up Resource and Dto, yes Dto is the generic one with none of the features on it but Resource is not

On the page that you mention I also see that

ContextableData provides the functionality to add context for includes and wraps to the data object/collectable
IncludeableData provides the functionality to add includes, excludes, only and except to the data object/collectable

It's not really mentionned that IncludeableData will not work without ContextableData, I think IncludeableData should extend the ContextableData in the next major because it doesn't work without it, same for the trait

https://github.com/spatie/laravel-data/blob/main/src/Concerns/IncludeableData.php#L13

@SethSharp
Copy link

hmm yes I see your point now. I was mixing up Resource and Dto.

@rubenvanassche
Copy link
Member

I've added it to Resource, IncludeableData earlier extended ContextableData but that gave circular dependency problems when trying to analyze data for TypeScript Transformer 3. This should fix the issue.

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 a pull request may close this issue.

3 participants