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

Bug with typescript transformation of Illuminate\Support\Collection in v4.4.1 #719

Closed
studnitz opened this issue Mar 26, 2024 · 4 comments
Closed

Comments

@studnitz
Copy link

studnitz commented Mar 26, 2024

I think this commit 66e2510 introduced a bug.

Example:

<?php

use Spatie\LaravelData\Data;
use Illuminate\Support\Collection;


class FooData extends Data
{
   public function __construct(
        /** @var Collection<int, SomeData> */
        public Lazy|Collection $collection,
    ) {
    }
}

Before v.4.4.1 this would result in the Typescript Type:

declare namespace App.Data{
export type FooData = {
collection?: Array<App.Data.SomeData>;
};
};

Now it results in a record type something like this:

declare namespace App.Data{
export type FooData = {
collection?: Record<int, App.Data.SomeData>
};
};

which is not correct because it still gets serialized to an Array by Laravel-data in the end of the day.

This then leads to many wrongly caused type errors.

Also note that using Collection<SomeData> as the phpdoc type is not the solution imo, because then PHPStan errors that it expects two generic parameters TKey and TValue.

@studnitz studnitz changed the title Regression with typescript transformation of `Illuminate Bug with typescript transformation of Illuminate\Support\Collection in v4.4.1 Mar 26, 2024
@Tofandel
Copy link
Contributor

Tofandel commented Mar 26, 2024

Mmm, it can technically be both because the collection can have keys, if those keys are not sequential (eg 2, 7, 9), it serializes to an object which is a Record, if the Collection has no keys then it serializes to an array, so the correct type would be

export type FooData = {
collection?: Array<App.Data.SomeData> | Record<int, App.Data.SomeData>;
};
};

But I agree, if the collection is keyed with int, it's much better DX to say it should be an array because that's what it's going to be in 99.9999% cases and in the case it's not we can just say Collection<int | string, SomeData> and the type should correctly be infered as a Record

@Tofandel
Copy link
Contributor

Tofandel commented Mar 26, 2024

@studnitz

Did you try with

/** @var Collection<SomeData> */

@studnitz
Copy link
Author

@studnitz

Did you try with

/** @var Collection<SomeData> */

Yes, but this results in a PHPStan error, see the last paragraph of the bug report.

@rubenvanassche
Copy link
Member

Is a bug indeed, fixed in the next version.

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

No branches or pull requests

3 participants