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

Fix payload serialization #115

Open
tonysm opened this issue Sep 3, 2023 · 4 comments
Open

Fix payload serialization #115

tonysm opened this issue Sep 3, 2023 · 4 comments
Milestone

Comments

@tonysm
Copy link
Collaborator

tonysm commented Sep 3, 2023

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
Copy link
Collaborator Author

tonysm commented Sep 3, 2023

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.

@tonysm tonysm added this to the 2.0.0 milestone Feb 7, 2024
@reshadman
Copy link

@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
Copy link
Collaborator Author

tonysm commented Jun 12, 2024

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
Copy link
Collaborator Author

tonysm commented Sep 13, 2024

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.

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

2 participants