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

Proxy refactoring #163

Merged
merged 1 commit into from
Feb 7, 2020
Merged

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Feb 5, 2020

As highlighted by #83, there are still performance issues with the proxy.
When a proxy is created, even if it's not directly assigned to a variable, it stays in memory and it keeps observing the variable. When accessing multiple time the same variable, the proxyHandler creates a new proxy object every time and they remain active after the initial use.
It means then, if we read 10 time a property containing an object, there will be 10 proxy watching the variable which will trigger 10 refreshed every time it changes.

Proposed new approach: rather than creating a new proxy every time we access a nested variable, we create a deep proxy at the beginning so we don't proxy only the first level but we do recursively for all level (objects would generate a proxies as soon as a directive use them any way so resource wise it's pretty much the same). The get trap just return true if property is '$isAlpineProxy' otherwise it returns the property value: we don't create new proxy here so it should resolve both the issues where proxy were nested or duplicated.
The set trap, when we add a new object, will try to create a deep proxy before inserting it to keep the property reactive.

The deepProxy utility will just create a proxy for valid objects and will ignore null values, DomNodes, existing proxies, etc.

I tried myself and I saw performance improvements, old tests still pass and examples looks okay.
I've asked one of the user to stress test it as well.

I'm conscious that it touches a low-level part of the core so I understand if you want to take a bit of time to test it or you want to changes/improves anything.

Hopefully it should fix all this proxy drama. :)

@SimoTod SimoTod requested a review from calebporzio February 5, 2020 20:31
@calebporzio
Copy link
Collaborator

I LOVE this. Thank you so much!

@calebporzio calebporzio merged commit 761a1a0 into alpinejs:master Feb 7, 2020
@SimoTod SimoTod deleted the feature/proxy-refactoring branch November 20, 2021 00:25
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.

2 participants