Attempt memory leak fix from using magics

[byronanderson]

From #2847 :

High frequency evaluations of alpine strings that use magics (requestAnimationFrame usage in our case) exacerbate a leak we have found.
What we observe is that _x_cleanups will grow by 1 every time you use a magic property in an evaluated string, which will eventually cause our web page to crash running out of memory.
Here is a minimal reproduction of the issue: https://codesandbox.io/s/alpine-memory-broken-mlip56?file=/index.html
Resize the preview window on that page and you should see that _x_cleanups for the div with id "leaky" grows.

We have two thoughts about potential fixes for this behavior in injectMagics function:

1. To memoize the setup/cleanup mechanism to only run once, lazily evaluated.
2. To clean up the previous call eagerly when setting up the new value.

We have an implementation for option 1 (this PR), but that assumes that the magic plugin function is only called once for a magic.

I.E. If we had a plugin that did something like this:

magic("myplugin", implementation1);
// some time later
magic("myplugin", implementation2);

The existing implementation would support this behavior, and option 2 would mitigate this downside, but require mutation.js to have a 'removeOnElRemovedCallback' kinda like this that we didn't quite know how to implement in fullness:

export function removeOnElRemovedCallback(el, callback) {
  el._x_cleanups = el._x_cleanups.filter(cb => cb !== callback);
}

Sorry I couldn't figure out how to get codesandbox to host my fixed version to illustrate :(

[SimoTod]

Do line 16-20 need to be inside the get definition? Can they just be moved to line 14 (inside the foreach bit outside the property definition?).

About redefining a magic, i don't think Alpine supporters it: magics are injected once when a component is evaluated the first time and those are the implementations that will be used. Anything defined after that moment is not visible to the component so I don't think it's a concern.

[byronanderson]

@SimoTod I think they need to be within get declaration to be lazily evaluated, so I think it would work if it was defined outside the get, but that would raise the overhead of using anything that uses injectMagics but doesn't actually use magics.

I'm going to think for a few minutes about other ways to lazily evaluate though, since I'm not sure that calling the callback once actually works for some cases in retrospect.

[byronanderson]

Edited: a previous version of this comment had a logic error in the memoization that caused a test failure, I fixed it and updated the comment to reflect newer information.

I'm worried about this PR breaking something because it assumes that the magic's callback will return a semantically identical value each time. I could bypass that by instead only lazily initializing whatever lines 16-20 do, which also bypasses the memory leak and passes tests:

export function injectMagics(obj, el) {
    Object.entries(magics).forEach(([name, callback]) => {
        let memoizedUtilities = null;
        function getUtilities() {
            if (memoizedUtilities) {
                return memoizedUtilities;
            } else {
                let [utilities, cleanup] = getElementBoundUtilities(el)
                
                memoizedUtilities = {interceptor, ...utilities}
                
                onElRemoved(el, cleanup)
                return memoizedUtilities;
            }
        }
        
        Object.defineProperty(obj, `$${name}`, {
            get() {
                return callback(el, getUtilities());
            },
            enumerable: false,
        })
    })

    return obj
}

[byronanderson]

I updated the PR to have the last comment's version of the patch, both commits on the branch fix the memory issue but in different ways, maybe the second commit is safer to roll out.

[byronanderson]

Opened an issue for the underlying behavior problem to track independently of potential fixes: #2847

[joshhanley]

Thanks for the PR! We're going to leave this one open for now so Caleb can have a close look at it when he gets the chance. He agrees that the current implementation is inefficient so wants to look at it in detail.

[joshhanley]

Sorry, hit close instead of comment! 🤦‍♂️

[joshhanley]

As a further follow up, we're going to leave this PR open for now, as Caleb wants to review it in detail, but currently doesn't have time whilst working on Livewire V3.

[calebporzio]

Thank you so much for this fix @byronanderson!