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

♻️ Cleans Up Entangle #3792

Merged
merged 4 commits into from Oct 18, 2023
Merged

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Oct 3, 2023

Just improves the logical flow of entangle to clean out unnecessary operations.

The whole stringify/parse thing seems unnecessary and there is nothing in the history to indicate why it was ever needed (and it didn't even do it in all cases). Seems like an unexpected behavior to have it clone the objects, and not explicitly reference the same object. I definitely wouldn't understand why using modelable with an object would not share references...It was just always rerunning the entangle if a property changed.

But this also allows an initial outer value of undefined which did not work before.

It can be reintroduced to actually clone (and do it for all cases) pretty simply, if you remember why that was a wanted behavior.

In fact, don't even need the hash really, could just directly reference the previous and compare.

@joshhanley
Copy link
Collaborator

@ekwoka thanks for the PR! The entangle code was originally in Livewire. There were issues with Alpine mutating Livewire's javascript objects without the parse. Here's a PR for reference livewire/livewire#2251

So we opted to clone the data using stringify/parse. Do you know if your changes will cause the same issue?

@ekwoka
Copy link
Contributor Author

ekwoka commented Oct 4, 2023

I can check.

Do you know of a reason that the outer being set to the inner before was NOT doing this cloning? It would just copy by reference.

That was the thing that made me more confused about if it was necessary, since surely if it was needed it should be done every time, not just some of the time.

I'll probably edit this to just do the clones all the time (but maintain the other cleanliness improvements) if I can't meaningfully validate that it's safe for Livewire.

I assume that maintaining compatibility with older LiveWire is still necessary?

@joshhanley
Copy link
Collaborator

joshhanley commented Oct 5, 2023

@ekwoka which part do you mean about the outter being set to the inner?

In regards to safe for Livewire, I can pull your branch into Livewire and run the test suite to confirm, once you think it's good to test.

This is only for Livewire V3, as Livewire V2 as it had this code built in, so don't need to maintain compatibility with V2.

@ekwoka
Copy link
Contributor Author

ekwoka commented Oct 5, 2023

@joshhanley Previously the branch for responding to changes did not clone the value when assigning it from the outer to the inner

if (outerHashLatest !== outerHash) { // If outer changed...
    inner = innerGet()
    innerSet(outer)

So this was only copying by reference.

It was only doing the deep cloning for the initial setting, and if the inner changed. This definitely struck as peculiar.

And no worries, I can do the livewire check myself. It's something I've been meaning to check out.

EDIT: It does fail the livewire tests, so I'll reintroduce the clone, and to be consistent, do so for all entangled assignments.

@calebporzio
Copy link
Collaborator

Thanks for checking that out in Livewire's codebase @ekwoka!

@calebporzio calebporzio merged commit 3184106 into alpinejs:main Oct 18, 2023
1 check passed
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.

None yet

3 participants