spatie / laravel-data

Powerful data objects for Laravel
https://spatie.be/docs/laravel-data/
MIT License
1.29k stars 211 forks source link

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

Closed Tofandel closed 1 month ago

Tofandel commented 1 month ago

✏️ 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 https://github.com/spatie/laravel-data/blob/cf86d7abc18437a68d34c50072e5951b4a0bcf98/src/Concerns/TransformableData.php#L35-L37

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'],
        ],
    ]);
});
SethSharp commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

rubenvanassche commented 1 month ago

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.