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

Allows access to methods when class instance used for x-data #4038

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Feb 15, 2024

Currently, if a class instance is used as the data scope, instance methods (and inherited properties shudders) are not accessible in expressions.

This is due to only checking for own properties in mergeProxies.

This changes this to use Reflect.has to better reflect the actual operation being performed, and uses that same behavior in other places doing this check for consistency. (Of note, I found in some testing that using Reflect.ownKeys in the ownKeys trap caused some very strange call stack issues).

(Note on using Class instances in general: actually private instance properties/methods will not be accessible from other methods on that class instance due to how the proxy reflection actually works).

@calebporzio
Copy link
Collaborator

Thanks for this contribution @ekwoka - can you give me more context into where the need arose for this? A GH discussion or something? Has there been issues faced by multiple people relating to this?

@ekwoka
Copy link
Contributor Author

ekwoka commented Feb 16, 2024

It was in the Discord. I think it came up twice there in the last 2 months.

I believe there was also a discussion here but not related directly to this as an issue, so I'm not positive how we'd find it.

I think the first commit of this PR is sensible even without it as a major concern, since it is less code and more appropriate use of Reflection.

But yes, the second and third are just about addressing some potential future issues, that may just not be very important since the likelihood of such issues is very very very low.

Side note: I think classes as data in Alpine have some interesting possibilities that are worth investigating. For high reuse components it would be more efficient than standard factory functions, and decorators could make things like interceptors a bit more ergonomic. I haven't explored it much, but this fix here would be necessary.

@calebporzio
Copy link
Collaborator

Mmmm yeah very interesting possibilities @ekwoka.

Do you mind removing the second commit then and we can tackle that as an independent task at some point? (sorry I'm OOO and will merge this if it's merge-ready) Thanks!

@calebporzio
Copy link
Collaborator

Thanks!

@calebporzio calebporzio merged commit ec11d59 into alpinejs:main Feb 18, 2024
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.

2 participants