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

(Improve extensibility) Export injectMagics, remove dontAutoEvaluateFunctions #4309

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ChrisVilches
Copy link

@ChrisVilches ChrisVilches commented Jul 20, 2024

Summary:

  • Export injectMagics to make it possible to make certain plugins.
  • Removed dontAutoEvaluateFunctions because runIfTypeOfFunction depends on data managed by that function, but runIfTypeOfFunction is not exported, therefore it's impossible to make certain plugins that need to change the evaluator.
  • Refactored 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)
  • Renamed runIfTypeOfFunction to handleEvalResult, 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.

I tried to sort the imports by length size but I didn't want to touch it too much.

Some extra info regarding injectMagics: This function injects into an object all the magics that are inside this array hidden inside the module. Since this array is inside the module (and not exported), it can't be accessed, and even if I copied the source code of injectMagics, I wouldn't be able to get the data. Therefore for plugin makers that need to inject magics, this is a requirement.


Additionally, injectDataProviders does an equivalent thing for the datas array (which gets populated when using Alpine.data(...)). I don't need this function for my plugins so I didn't export it, but may be for someone.

@ekwoka
Copy link
Contributor

ekwoka commented Jul 20, 2024

While I'm generally in favor of making more internals exposed, I am quite curious (and concerned) about what in your plugin you feel warrants access to these specifically.

Right now runIfTypeOfFunction isn't used in any internal directives or magics, and injectMagics is only in one (x-data).

They don't have particularly clear utility outside of where they are used, so I immediately worry about the approach your plugin is taking that makes these appear necessary.

Separately there is the concern that, at least for runIfTypeOfFunction, it's quite internal in the sense of "What is the real value to making this public as far as any guarantees about it's behavior and signature?".

injectMagics doesn't quite have that problem, and has a bit more utility.

But anway.

Yeah, if you have something demonstrating the plugin you're trying to make, it will be easier to advise on this.

@ChrisVilches
Copy link
Author

ChrisVilches commented Jul 20, 2024

@ekwoka runIfTypeOfFunction has the same "problem" as injectMagics in the sense that it uses data that is hidden inside the module: the dontAutoEvaluateFunctions and its relationship with shouldAutoEvaluateFunctions.

If this didn't happen, then I could just write my own function runner, and it'd be OK. So if I understand these dependencies correctly, I need to extract the one that comes from evaluator.js and use that one.

By the way, my plugin is just a tweaked evaluator (that's why setEvaluator is public anyway I guess 😉). I can get the closestDataStack, and mergeProxies from the Alpine object in order to populate the scope, but I need the missing ones (the ones I added).

In my opinion it'd be better to pass a flag to the evaluate function that tells it to not run functions automatically, then get rid of dontAutoEvaluateFunctions (it's only used in one place), but mind this function has already been public and perhaps other people are using it in their plugins?

@ChrisVilches
Copy link
Author

ChrisVilches commented Jul 20, 2024

A more complicated alternative would be to deprecate dontAutoEvaluateFunctions, and then the only usage of this function (in /utils/bind.js): (update: actually there is more than one usage)

        return dontAutoEvaluateFunctions(() => {                                                                                                                                      
            return evaluate(el, binding.expression)
        })

Change it to:

evaluate(el, binding.expression, {}, false) // <--- extra flag for auto execution

Then we can remove dontAutoEvaluateFunctions, and runIfTypeOfFunction becomes free of dependencies to data hidden inside the module (and I wouldn't need it anymore since the auto execution flag would be fed directly to the evaluator function).

@ekwoka
Copy link
Contributor

ekwoka commented Jul 20, 2024

I enjoy this rethought approach. Solve the problem with less code :)

@ChrisVilches ChrisVilches changed the title (Improve extensibility) Export injectMagics and runIfTypeOfFunction (for plugin makers) (Improve extensibility) Export injectMagics, remove dontAutoEvaluateFunctions Jul 20, 2024
@ChrisVilches
Copy link
Author

@ekwoka I did the refactors I was talking about. Please check them out.

By the way, the sort file was changed, but all tests are pending, therefore we don't know if something broke:

npx cypress run --quiet --spec tests/cypress/integration/plugins/sort.spec.js

...

10 pending

I manually tested it with the examples in the doc, and they worked.

@@ -117,7 +103,7 @@ function generateEvaluatorFromString(dataStack, expression, el) {
// Check if the function ran synchronously,
if (func.finished) {
// Return the immediate result.
runIfTypeOfFunction(receiver, func.result, completeScope, params, el)
runIfTypeOfFunction(autoEvaluateFunctions, receiver, func.result, completeScope, params, el)
Copy link
Contributor

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.

Copy link
Author

@ChrisVilches ChrisVilches Jul 21, 2024

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 another async function, then it will continue executing it recursively. In other words, when it's async, it will unwrap all the promises and return the final value.

Check this example.

<html>
<head>
    <script defer src="https://cdn.jsdelivr.net/npm/[email protected]/dist/cdn.min.js"></script>
</head>
<body>
    <h1 x-data="{ fn: () => () => () => () => 5 }" x-text="fn"></h1>
    <h1 x-data="{ fn: async () => async () => async () => async () => 5 }" x-text="fn"></h1>
</body>
</html>

Result on screen:

() => () => () => 5
5

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 considering runIfTypeOfFunction is contained (private) inside evaluator.js (alpinejs/csp).

Copy link
Contributor

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...

Copy link
Author

@ChrisVilches ChrisVilches Jul 22, 2024

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.

  • Refactored 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)
  • Renamed runIfTypeOfFunction to handleEvalResult, 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:

export function handleEvalResult(autoEvaluateFunctions, receiver, value, scope, params, el) {
    const shouldCallFunc = autoEvaluateFunctions && typeof value === 'function'

    const result = shouldCallFunc ? value.apply(scope, params) : value

    if (result instanceof Promise) {
        result.then(i => receiver(i)).catch( error => handleError( error, el, value ) )
    } else {
        receiver(result)
    }
}

Copy link
Contributor

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? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

like

const inner receiver = autoCall 
  ? (maybeFn) => maybeFn instanceof Function 
    ? receiver(maybeFn.apply(scope, params)) 
    : receiver(maybeFn) 
  : receiver

well, prettier than that I mean...

Copy link
Author

@ChrisVilches ChrisVilches Jul 23, 2024

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):

    return (receiver = () => {}, /* ... */) => {
        // ....
        // modify the receiver here

        handleEvalResult(...modifiedArgs, result)
    }

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?

Copy link
Contributor

@ekwoka ekwoka Jul 23, 2024

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.

Copy link
Author

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

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.

Making things better by removing code :)

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