laravel-data
laravel-data copied to clipboard
spatie
feat: support optional properties
Original PR: https://github.com/spatie/laravel-data/pull/127 (was closed due to the deletion of the v2 branch)
This PR is a second take at https://github.com/spatie/laravel-data/pull/119, that does not replace the default behavior but supports the newly configurable one.
It depends on https://github.com/spatie/typescript-transformer/pull/30 and https://github.com/spatie/laravel-typescript-transformer/pull/15. Both of these should be merged before this PR (tests will not pass anyway).
What's important:
- The
#[Optional]attribute fromtypescript-transformeris taken into account - The new
transform_null_to_optionaloption fromlaravel-typescript-transformeris taken into account - The current behavior does not change unless one of the above is true
Example:
use Spatie\TypeScriptTransformer\Attributes\Optional;
class UserData
{
public function __construct(
#[Optional]
public int $id,
public string $first_name,
) {
}
}
interface UserData {
id?: number
first_name: string
}
The Optional attribute above would not be necessary if config/typescript-transformer.php's transform_null_to_optional is set to true (which is false by default to preserve current behavior).
@innocenzi - Why would this not be based on whether the property is nullable?
Example:
class UserData
{
public function __construct(
public ?int $id,
public string $first_name,
) {
}
}
@aidan-casey This PR does that, if transform_null_to_optional is enabled (which it is not by default). The maintainers of laravel-data do not want this to be the default behavior, see https://github.com/spatie/laravel-data/pull/119 for details.
@innocenzi - Sorry, I phrased that in a confusing way. My question is more: if a property is nullable in PHP, why would it not just be nullable in Typescript as well? In Typescript and PHP null <> optional.
I explained it in the discussion of the PR linked, but here's another explanation.
In PHP, there's no such thing as "optional". TypeScript is different; there is null and undefined.
The difference is that null means that a variable has been assigned an empty value (null), while undefined means a variable has not been assigned any value (though you can assign undefined to a variable).
Practically speaking, this means you cannot omit nullable properties on an object in TypeScript. This leads to poor DX and require the use of custom generic types to transform nullable properties of a type to optional ones in order to avoid being forced to initialize them all with null, which is both cumbersome and not useful.
The TL;DR is that "nullable" in PHP translates better to "optional" in TypeScript than a null type union.
@innocenzi - My point is that I don't think that is an accurate equivalent though. Marking a property as nullable in PHP does not make it optional. You still have to pass a value. This aligns perfectly with Typescript's nullable as well.
For example:
// This class would require either null OR a string to be passed, but cannot be omitted.
class Test
{
public function __construct(?string $value)
{}
}
The only time in which this package will automatically set to null is when building from request data outside the construct. We could argue whether or not it should do that, but when using traditional PHP syntax the above would throw an error if nothing was passed.
I know and understand that, however my points regarding DX still stand. If you want, I can give you code snippets tomorrow that demonstrate how annoying it is to work with nullable properties in a TypeScript interface.
Working with PHP and working with TypeScript are two different things. The type system is different, and we don't write the same kind of code (back-end logic and front-end manipulation). My point there is that it is not right nor practical to try to directly translate one language's way of working to another. The goal is to make them interoperate nicely, in a way that makes sense.
Please keep in mind that this PR brings this functionality as an option, disabled by default. If you don't want this behavior, you don't have to deal with it (though I'm really not sure why one would like to deal with all null union types in interfaces with potentially dozens of properties).
Just a heads up that upstream PRs are in a ready-to-be-reviewed state, they provide value without adding too much code, and if I can help in any way to make this move forward you can let me know. 😊
Sorry for pushing, I know that you are busy - just making sure this doesn't fall into oblivion.
While I understand it makes sense from a practical point of view, I can't help but shake the feeling that it's "incorrect" :)
I get that adding #[Optional] to every property is a hassle, would and #[OptionalProperties] attribute to declare all properties of the class optional more ergonomic? That'd give the developer more control than a global assumption.
Another idea is to mark properties that have an explicit null value as default as optional. Also not 100% correct, but it aligns well with the idea of "these properties don't need to be provided explicitly".
class User extends Data
{
public function __construct(
public string $email,
public ?string $name,
public ?int $age = null,
) {}
}
interface User {
email: string;
name: string | null;
age?: number;
}
Hey @sebastiandedeyne, thanks for the answer! Can you elaborate on your feeling that it's incorrect? Maybe I can clarify somehow why I think it's actually the correct behavior.
I think that you feel this is incorrect because with that option enabled, you would have no way to mark a property as nullable instead of optional? Or is it because you definitely feel like a nullable property in PHP translates well into a nullable property in TypeScript?
That'd give the developer more control than a global assumption.
We can always offer an escape hatch with something like a #[Nullable] attribute:
class User extends Data
{
public function __construct(
public string $email,
public ?string $name,
#[Nullable]
public ?int $age,
) {}
}
interface User {
email: string;
name?: string;
age: number | null;
}
But I really think that would just solve an edge-case - this is never something I needed or wanted to do, personally. I avoid null as much as I can in TypeScript, there's only a few cases where it's needed.
That being said, among your suggested alternative, my favorite would be the #[OptionalProperties] attribute assigned to the Data Object.
Hey, I wanted to request the same feature as added here - an Optional attribute instead of the Optional object I have to type and assign.
Is there any ETA for this PR and a corresponding release?
Right now we have DTOs like the following and it just feels wrong to type a package class to make it optional if the reality is that this property should be null if directly accessed and removed if transformed to array. As also in application code doing $dto->id instanceof Optional instead of $dto->id === null feels extremely wrong.
final class Transaction extends Data
{
public function __construct(
public readonly Optional|int $id = new Optional(),
public readonly Optional|string $code = new Optional(),
public readonly Optional|int $companyId = new Optional(),
) {}
}
Laravel Data
Ruben & I discussed this today. Here's what we've decided for now:
We're going to add support for your proposed Optional attribute.
class User extends Data
{
public function __construct(
public string $email,
public ?string $name,
#[Optional]
public int $age,
) {}
}
type User = {
email: string;
name: string | null;
age?: int;
}
When using Laravel Data's Optional type, TypeScript transformer will behave the same.
class User extends Data
{
public function __construct(
public string $email,
public ?string $name,
public int|Optional $age,
) {}
}
We're going to hold off on a global flag to change the behavior of undefined properties in TypeScript. Instead, we'll allow Optional to be used on the entire class.
#[Optional]
class User extends Data
{
public function __construct(
public string $email,
public ?string $name,
public int $age,
) {}
}
type User = {
email?: string;
name?: string | null;
age?: int;
}
We're holding off on the rest because in the coming months we're planning to experiment wit data objects setups with optional properties and more, and don't want to rush in with an API yet.
So you can expect more improvements in the future, but we'll need some more time :)
Ruben is currently updating this PR to match the above, should be merged soon!
Fantastic news! Thank you very much for advancing on this topic. Looking forward to what you'll come up with regarding your experiments, and in the meantime the class attribute will be muuuch better than my current workarounds.
Let me know if I can be of any help!
I just realized, when trying to upgrade to this PR from my workaround, that the #[Optional] attribute makes all attributes optional, instead of just nullable ones... which really is not useful in the scenario I was trying to solve with this PR in the first place. :(
Example in the wild: I really don't want null in my types. It simply has poor compatibility with the front-end world.
We're holding off on the rest because in the coming months we're planning to experiment wit data objects setups with optional properties and more, and don't want to rush in with an API yet.
Hey, did you guys had time for this yet? I'd love to help — currently, #[Optional] on a class fixes an issue but creates another one by making type-checking useless for required properties :|
I still think my original proposal is a good compromise and I don't think a breaking change would be bad if you find a better solution later.