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

feat!: add context to transformers #589

Closed
wants to merge 2 commits into from

Conversation

innocenzi
Copy link
Contributor

This pull request is an alternative to #588, which I personally prefer.

Along with also allowing multiple transformers for a class, it adds a context property to the Transformer contract, which can be an arbitrary string.

This allows to transform data differently depending on the context (response, eloquent, a custom context...).

Unfortunately, this is a breaking change if using a custom Transformer implementation. I couldn't find a way to implement the feature in a backwards-compatible way.

Example  

Usage:

// In a controller
return SomeData::collection($query->paginate())->transform(context: 'hybridly');
// or
return SomeData::from($data)->transform(context: 'hybridly');

App-specific stuff:

// Custom TypeScript Transformer that generates a proper type using `HasOptions`
final class OptionsTransformer extends EnumTransformer
{
    protected function toEnumValue(ReflectionEnumBackedCase $case): string
    {
        if ($case->getEnum()->implementsInterface(HasOptions::class)) {
            return json_encode($case->getValue()->toOption());
        }

        $value = $case->getBackingValue();

        return \is_string($value) ? "'{$value}'" : "{$value}";
    }
}

// Custom Laravel Data transformer for transforming the enum in options
final class OptionsTransformer implements Transformer
{
    public function transform(DataProperty $property, mixed $value, ?string $context = null): null|string|array
    {
        if (!$value instanceof BackedEnum) {
            return null;
        }

        if ($context === 'hybridly' && $value instanceof HasOptions) {
            return $value->toOption();
        }

        return $value->value;
    }
}

// Interface for my enums
interface HasOptions
{
    public function toOption(): array;
}

// Example implementation
enum PilotGrade: string implements HasOptions
{
    case CAPTAIN = 'captain';
    case FIRST_OFFICER = 'first_officer';
    case PILOT = 'pilot';

    public function toHumanReadableString(): string
    {
        return match ($this) {
            static::FIRST_OFFICER => 'First officer',
            static::CAPTAIN => 'Captain',
            static::PILOT => 'Pilot',
        };
    }

    public function toOption(): array
    {
        return [
            'value' => $this->value,
            'label' => $this->toHumanReadableString(),
        ];
    }
}

Concerns:

  • $context as a string might be restrictive, this could be an array or even a dedicated class
  • Calling transform with the same context everywhere would be repetitive, but this could be attenuated by making the BaseData class Macroable
  • This is a breaking change so we have to wait until v4... :(

@rubenvanassche
Copy link
Member

Indeed something for v4

A few remarks:

  • $context as a string might be restrictive, this could be an array or even a dedicated class

I think a more dedicated class here is the way to go, allows us to have a more enum like context (Array, Response, Json, Model). Would keep it open for users to define a custom context when those four aren't enough.

  • I'm seeing that it would be possible to have multiple data transformers per property, I think transforming data is already slow. Adding multiple transformers even would make it more complicated. I would keep it as is, a transformer is allowed to output different values based upon its ingested context.

  • Calling transform with the same context everywhere would be repetitive, but this could be attenuated by making the BaseData class Macroable

I hate macroable, in the case you want such a thing, then creating a custom data class from which all your own data classes extend is the way to go

Probably going to merge this in v4 but I also want to check with the Spatie what they think about this.

@innocenzi
Copy link
Contributor Author

Hey, thanks for checking the PR out!

I think a more dedicated class here is the way to go

That's my preference as well, but wanted to keep the initial PR simple 👍

I'm seeing that it would be possible to have multiple data transformers per property, I think transforming data is already slow. Adding multiple transformers even would make it more complicated. I would keep it as is, a transformer is allowed to output different values based upon its ingested context.

I understand the concern, but this is kind of required in order to avoid rewriting all existing transformers. In most situations, there would be only one transformer only anyway. Perhaps we can benchmark it?

I hate macroable, in the case you want such a thing, then creating a custom data class from which all your own data classes extend is the way to go

Agreed 👍

@innocenzi
Copy link
Contributor Author

innocenzi commented Feb 9, 2024

Looks like v4's TransformationContext might help with this feature? 👀

EDIT: I'll have to take a closer look, but I'm pretty sure I can do what I wanted thanks to withGlobalTransformer :D

@rubenvanassche
Copy link
Member

Great to hear, not sure if we want an array for context but maybe that's really useable. Was also thinking about adding a TransformationOutput enum or such a thing with values : array, Model, response, json, ... not yet sure about the exact implementation.

As for this PR I'm gonna close it for now since it has a lot of conflicts and the issue seems resolved.

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.

2 participants