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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add failing test for array update hooks $value #8235

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshhanley
Copy link
Member

@joshhanley joshhanley commented Mar 31, 2024

This PR adds a failing test for handling of update hooks for array properties (and possibly others).

The test is structured the way that V2 handles update hooks for arrays.

See discussions #7101 and #6665.

The Scenario

When properties are updated on the front end, Livewire triggers updatingProperty and updatedProperty hooks on the backend (plus updating/ updated but they aren't needed for this example).

This allows developers to hook into the updates and perform other actions or side effects based on the values in the update hooks.

In particular, the updatingProperty hook allows your to inspect the new property value before the property has been updated.

When using hooks in V2, the $value passed into the hook was always the new value that a property is going to be set to and there was no $key passed in.

For example if an array gets updated by the front end (such as when using entangle), then that value is passed to the update hooks as the $value prop.

So if you had a property with an array ['one', 'two', 'three', 'four'] and it gets changed on the front end, to ['two', 'three'], then the new value would be sent to the backend and the $value prop on the update hooks would contain ['two', 'three'].

Pasted image 20240331153421

It is to be noted that each of the updatingProperty/updatedProperty hooks would only be called once each for a single property change.

See below screen capture of this in action in V2.

Recording 2024-03-31 at 14 53 55

The Problem

The problem is that in V3 the way the front end handles updating properties has changed.

Instead of sending the new value of a property to the backend, Livewire will now calculate the changes (diff) to a property (particularly when they are arrays/objects) and send each of the diffs as individual update calls.

Pasted image 20240331153447

What this means, is that for every update that happens to a property, each of the updating/updated hooks will be called.

In the screenshot above, this means for the items property updatingItems and updatedItems hooks will each be called 4 times.

Each time the update hooks are called the $value that is being passed in is the values in each of the updates, so for the first hooks call it's "two", then second is "three", and then "__rm__" and then "__rm__". And the $key is now populated with the corresponding index of the update 1, then 2, then 3, then 4.

For users that were relying on this functionality in V2 this is an unexpected change.

It also means the internal __rm__ value is being exposed, meaning a developer has to interact with it and decide what to do with it, which is not ideal.

See below screen capture of this in action in V3.

Recording 2024-03-31 at 14 58 41

Solutions

Solution A) Change the front end

We could go back to how the front end handles a property update when it gets sent to the server like in V2.

Pros

  • Makes dealing with it on the backend really simple as there will only be one update for a property, so the appropriate hooks can be run once.

Cons

  • Caleb has previously said that calculating the diffs on the front end is a better idea (for personal curiosity reasons - I'd love to know why that is the case 馃榿).
  • Could require signficiant rework of the front end to get that to happen.

Solution B) Only use the diff to determine if a request should be sent, but use the whole value for the update

This depends on the reasoning for the diff in the first place. My suspicion is that calculating the diffs is helpful for determining whether a request should even be sent at all. But we could still do that for handling of the request, but then send the new value as a single update call (not push the diff into the updates).

Pros

  • In theory seems simpler than reworking all of the front end.

Cons

  • May not be helpful if there is actually benefit to sending the diffs as part of the request to the backend.

Solution C) Aggregate the changes on the backend

We could group all the changes for a particular property together on the backend and make the update to the property with the final value of the property - thus calling the update hooks once with the final value.

Pros

  • Means a single set of update hooks will be run per property change.

Cons

  • The code for this might not be great to maintain.
  • What happens if the list of updates also has updated for properties nested under this property?

Solution D) Do nothing

These changes have been in effect for a while now. So we could just leave it the way it is.

Pros

  • Easy as there is nothing to do.

Cons

  • Multiple update hooks run, which could lead to performance issues if it is a large array.
  • __rm__ is still exposed to the developer.

Recomendation

I think if I were to choose, I would probably lean towards solution B, as it could be the best of both worlds. I hold this opinion loosly though as I don't have the full context on why diffs are good.

@ITSYV
Copy link

ITSYV commented Mar 31, 2024

Is this failing test related to this case as well (checking differences on updates): #6905 ?

@joshhanley
Copy link
Member Author

joshhanley commented Apr 2, 2024

@ITSYV nope that looks like a separate problem to me. Any chance you could submit a failing test PR for that one? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants