hotwired-laravel / turbo-laravel

This package gives you a set of conventions to make the most out of Hotwire in Laravel.
https://turbo-laravel.com
MIT License
803 stars 50 forks source link

Fix payload serialization #115

Open tonysm opened 1 year ago

tonysm commented 1 year ago

When we send a broadcasting job to be processed in background, because the job payload can be anything (whatever you want to pass to the view), we don't really use Laravel's ModelIdentifier class (here).

The current implementation works, but the jobs are unnecessarily big, since the object is serialized using PHP's normal serialization. This also has the side-effect of the model having all the attributes it had during from the moment the model was serialized. For instance, if we wanted to return the HTML slightly modified as an HTTP response Turbo Stream for the user that created the resource, we'd use the $model->wasRecentlyCreated, but if we wanted the broadcasted Turbo Stream to not be modified, we would have to override the data passed to the view, because since the model was serialized, it will also have the $model->wasRecentlyCreated set to true when the broadcast job is processed.

I think this could be solved by walking through the data array and finding out the eloquent models and either converting them to either use ModelIdentifier or maybe using globalid-laravel (here).

tonysm commented 1 year ago

I just tested using Laravel's SerializableClosure, but I don't think it's gonna work, unless we change the API.

For this package to work, the API would have to be something like this:

$todo->broadcastAppend()
  ->view(fn () => view('todos._todo', ['todo' => $todo]))
  ->later();

Notice the fn () => view(/** ... */) Closure passed to the view. This way, the Closure would be correctly serialized. default. This is because the serializable-closure package won't serialize it correctly like this:


$data = [
  'todo' => $todo,
];

serialize(new SerializableClosure(fn () => view('todos._todo', $data)));

We could probably handle the default behavior inside the package itself (serializing the model correctly.

Also, using the package we probably wouldn't be able to dispatch a queued job in a REPL context like tinker (or Tinkerwell), since it won't work in those envs.

reshadman commented 4 months ago

@tonysm The call to the $comment->broadcastReplace(); couldn't be moved to the job? instead of all of the parameters needed to build TurboStreamBroadcast ?

tonysm commented 4 months ago

Not sure I get your suggestion. 🤔 The issue is not about the broadcast methods themselves. The problem is only when we pass custom views with data since we need to serialize that to render on the queued job. If we're only broadcasting the model itself, then this is not an issue, since it serializes just fine.

tonysm commented 1 month ago

I think Laravel's new Defer helpers would work here instead of pushing rendering to a queued job... maybe the entire "later" publishing could use defer instead of queued jobs.