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

fix: memoization when injecting magic #4276

Merged
merged 6 commits into from
Jul 1, 2024
Merged

Conversation

AbhiShake1
Copy link
Contributor

@AbhiShake1 AbhiShake1 commented Jun 26, 2024

description

previously, the variable that was supposed to hold the value was being recreated for every loop. this has now been moved up to the hierarchy of dependencies. also, some minor edits have been made to make the code more readable and less error prone to further edits (subject to reviews)

Copy link
Contributor

@ekwoka ekwoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

It seems like getUtilities can be moved entirely outside injectMagics and just also accept the el as an argument instead.

@AbhiShake1
Copy link
Contributor Author

AbhiShake1 commented Jun 27, 2024

Great!

It seems like getUtilities can be moved entirely outside injectMagics and just also accept the el as an argument instead.

Thanks. I am not sure if the extracted function output should be cached where its cached now or maintain a global cache map. What would you recommend?

Edit:
Well actually, we dont need to explicitly cache at all

@ekwoka
Copy link
Contributor

ekwoka commented Jun 27, 2024

Uh, I think it should probably be cached on the element itself...if we were to redesign it more.

I jsut mean "getUtilities" as a function doesn't need to be scoped inside there. It it only references el and that can be passed it, so we don't need to create getUtilities over and over

@AbhiShake1 AbhiShake1 requested a review from ekwoka June 27, 2024 10:13
@AbhiShake1
Copy link
Contributor Author

Uh, I think it should probably be cached on the element itself...if we were to redesign it more.

I jsut mean "getUtilities" as a function doesn't need to be scoped inside there. It it only references el and that can be passed it, so we don't need to create getUtilities over and over

uhh i just overcomplicated it in my head. extracted

@calebporzio
Copy link
Collaborator

I refactored this a bit to simplify (the method was extracted but not used elsewhere, so I brought it back in and also tweaked the code style to conform to the rest of the project).

Are you both good with this before I merge?

@AbhiShake1
Copy link
Contributor Author

AbhiShake1 commented Jun 27, 2024

I refactored this a bit to simplify (the method was extracted but not used elsewhere, so I brought it back in and also tweaked the code style to conform to the rest of the project).

Are you both good with this before I merge?

I had moved it to a different file since that function isnt a direct concern of magic. Thanks for tweaking the code style. Also, does using const instead of let violate coding standards of this repo in any way?

@ekwoka
Copy link
Contributor

ekwoka commented Jun 27, 2024

Looks good @calebporzio

@AbhiShake1 While some (like me) may disagree, Alpine is a prefer-let repo. Similarly, functions that are not actually used multiple places are normally kept in the same file as the code that it relates to. Here that would either be injectMagics or getElementBoundUtilities but not somewhere else. Thats the concept of "single file feature".

@AbhiShake1
Copy link
Contributor Author

Looks good @calebporzio

@AbhiShake1 While some (like me) may disagree, Alpine is a prefer-let repo. Similarly, functions that are not actually used multiple places are normally kept in the same file as the code that it relates to. Here that would either be injectMagics or getElementBoundUtilities but not somewhere else. Thats the concept of "single file feature".

Thanks for explaining. Now it sounds right for this repo. Good to merge from my end

@calebporzio calebporzio merged commit ac63592 into alpinejs:main Jul 1, 2024
1 check passed
@calebporzio
Copy link
Collaborator

Thanks everyone!

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