-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
(Improve extensibility) Export injectMagics
, remove dontAutoEvaluateFunctions
#4309
Open
ChrisVilches
wants to merge
4
commits into
alpinejs:main
Choose a base branch
from
ChrisVilches:add-exports-extensibility
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
09d0ed8
Export runIfTypeOfFunction and injectMagics so they can be used in pl…
ChrisVilches 609aaa6
attempt to sort imports by string size
ChrisVilches 8f5086d
remove dontAutoEvaluateFunctions, refactor, evaluator, and remove exp…
ChrisVilches 970211a
Renamed runIfTypeOfFunction to handleEvalResult
ChrisVilches File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a clean way to avoid calling this entirely if we aren't auto evaluating functions?
Since why call
runIfTypeOfFunction
if we aren't auto evaluating it, right? Then the function name doesn't make much sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekwoka
You mean to conditionally execute it if it's a function, else simply return the value?
One thing to mind is that
runIfTypeOfFunction
will execute the function once, and return whatever value it is (including another function), but if it returns anotherasync
function, then it will continue executing it recursively. In other words, when it'sasync
, it will unwrap all the promises and return the final value.Check this example.
Result on screen:
So
runIfTypeOfFunction
not only changes with different values of the auto-execute flag, but also whether the values are promises or not. I don't feel confident enough to decide how to refactor this, because maybe some part of the code needs the recursive unwrapping (although I've never seen something like this in the source code). I'd personally just leave it like that and refactor it next time, which wouldn't be that hard consideringrunIfTypeOfFunction
is contained (private) insideevaluator.js
(alpinejs/csp).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write it in the first place, but I feel I can say with high confidence that the case of functions returning promises that resolve to functions that return promises that resolve to functions was never a case that came up when writing that code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekwoka I refactored and renamed the function.
runIfTypeOfFunction
to remove the recursive behavior. Now the behavior is more predictable: it just evaluates once without recursively unwrapping functions (which by the way only worked for promises, which made it even harder to understand what it was doing)runIfTypeOfFunction
tohandleEvalResult
, because the name may not make sense anymore. Basically the purpose of this function is to send a result to the receiver, so this name is more generic I guess.Code snippet:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's looking a lot better.
But I think we go further!
What if being passed to handle eval result, the autoEvaluateFunctions prop just wrapped the
receiver
? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like
well, prettier than that I mean...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekwoka Thank you for the clarification! I think the problem is that this would make other codes more verbose. The wrapping logic would need to be done in the caller, and currently there are several places where
handleEvalResult(...)
is called, so all those places would need to get modified.Basically all places that look like this (three in total):
So I guess in this case it'd be better to just let
handleEvalResult
do the dirty job of deciding whether to execute or not. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Just exploring.
I guess it being the default case to auto evaluate makes it clunkier to abstract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekwoka Yes, I think it's a bit difficult to abstract it further (without doing a major revamp)
So from me that's all I have to add to the PR, it achieves everything I wanted, with only exporting
injectMagics
and by refactoring the other involved parts (handleEvalResult
function)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍