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

x-for patch #151

Closed
wants to merge 3 commits into from
Closed

x-for patch #151

wants to merge 3 commits into from

Conversation

SimoTod
Copy link
Collaborator

@SimoTod SimoTod commented Jan 30, 2020

Closes #144

This PR enables the reactivity in an x-for element when an array contains primitive types and an item is updated from the item context using x-model or something like x-on="item = '...'"

Note that it was already working when an array contained objects because something like item.name evaluates item first which would return a reference to the proxy item defined in x-data.

extraVars is now proxied so any attempts of setting one of its variables can be captured and we can re-route the assignment to the parent level.

To make it possible, I had to remove a few spread operators because the proxy object has to be propagated through the function chain and spreading a proxy would transform it back to a normal object.

Code is less elegant now but, on the sunny said, it seems that babel likes it better and the overall size of the bundle went down a bit so it looks like a win-win.

I believe there is also a little performance impact because I also had to add another with operator in the saferEval functions (Same reason as above, the spread operator needed to be removed).

@calebporzio
Copy link
Collaborator

Thanks for the PR @SimoTod,

I see hmm, removing the spreads are annoying, but I get it.

For some reason, this feels like an implementation that'll lead to more headaches. There are so many gotchas with Proxies, and I feel like we are handling them as we go in the main observableData proxy, but adding another one makes me nervous and I feel like it'll be a rabbit hole, what do you think?

Are any other implementations possible? Do you feel this way? Do you have reasons I'm wrong? Thanks!

@SimoTod
Copy link
Collaborator Author

SimoTod commented Feb 4, 2020

@calebporzio Yeah, I see that there are still many pending issue related to the proxy but it basically the core of Alpine and I'm not sure there's another way to support it.

This is what I think happens at the moment:

Starting from the happy scenario

x-data={items: [{name: 'one'}]} translates to Proxy[ items: Proxy[name: 'one'] ]
In a for loop, the item receives Proxy[ items: Proxy[name: 'one'] ] + [item: Proxy[name: 'one']]
Accessing item would then return a proxy for the original element so setting an object property such as name would trigger refresh and reactivity. All good for now.

But

x-data={items: ['one']} translates to Proxy[ items: 'one' ]
In a for loop, the item receives Proxy[ items: Proxy[name: 'one'] ] + [item: 'one']
Accessing item would return the value "one" so setting item will just replace the value of the variable in extraVars, not in the data context.
Since variable is just a copy of the original one, an external update would overwrite it and, since it's not proxied, an internal update wouldn't trigger a refresh.

My thoughts:

The new proxy doesn't have a get trap, it always return the original value (regardless it being an object, a primitive type, a symbol or a function) so it shouldn't add any headaches. The only trap is on the setter where we just capture an attempt to modify the item and we forward the change to the original object.
When items are objects and we set an item property, i.e. item.name = '...', it translates to "get item" (new proxy doesn't affect it, it will return the original proxy as per my first example) and then "set name" (already covered by the original implementation).

The double "with" is obviously a performance drop when you try to access a variable/function in the global context (not a bad one, it will do 3 lookups rather 2 but one could argue that the first "with" is already a performance drop). Using the right order, it could probably allow a generic $dispatch as described in #143.

About the spread operator, it's more elegant but it also create copies every time so, performance wise, the alternative is slightly better. There are tests in place which should prevent us to reintroduce it in the future.


Or we can decide not to support it and we update the doc to make it clear. It would also be good to print a console warning when someone tries to do it. As an alternative path, since we will support the index variable soon, we can suggest to use items[index] but it's not as nice as the other sintax.

<div x-data="{items: ['foo', 'bar']}">
    <template x-for="(item, index) in item">
        <span x-text="item"></span>
        <input x-model="items[index]">
    </template>
</div>

@SimoTod
Copy link
Collaborator Author

SimoTod commented Feb 7, 2020

Closing this PR while exploring alternative solutions

@SimoTod SimoTod closed this Feb 7, 2020
@SimoTod SimoTod mentioned this pull request Feb 7, 2020
@SimoTod SimoTod deleted the feature/x-for-patch 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.

use and x-for item with x-model
2 participants