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.

Sign up for GitHub

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

Jump to bottom

(Improve extensibility) Export injectMagics, remove dontAutoEvaluateFunctions #4309

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

(Improve extensibility) Export injectMagics, remove dontAutoEvaluateFunctions #4309

Show file tree
Hide file tree
Changes from 1 commit
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 Jul 20, 2024
609aaa6
attempt to sort imports by string size
ChrisVilches Jul 20, 2024
8f5086d
remove dontAutoEvaluateFunctions, refactor, evaluator, and remove exp…
ChrisVilches Jul 20, 2024
970211a
Renamed runIfTypeOfFunction to handleEvalResult
ChrisVilches Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/alpinejs/src/alpine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { mapAttributes, directive, setPrefix as prefix, prefix as prefixed } fro
import { start, addRootSelector, addInitSelector, closestRoot, findClosest, initTree, destroyTree, interceptInit } from './lifecycle'
import { onElRemoved, onAttributeRemoved, onAttributesAdded, mutateDom, deferMutations, flushAndStopDeferringMutations, startObservingMutations, stopObservingMutations } from './mutation'
import { mergeProxies, closestDataStack, addScopeToNode, scope as $data } from './scope'
import { setEvaluator, evaluate, evaluateLater, dontAutoEvaluateFunctions, runIfTypeOfFunction } from './evaluator'
import { setEvaluator, evaluate, evaluateLater } from './evaluator'
import { transition } from './directives/x-transition'
import { clone, cloneNode, skipDuringClone, onlyDuringClone, interceptClone } from './clone'
import { injectMagics, magic } from './magics'
Expand All @@ -27,12 +27,10 @@ let Alpine = {
get raw() { return raw },
version: ALPINE_VERSION,
flushAndStopDeferringMutations,
dontAutoEvaluateFunctions,
disableEffectScheduling,
startObservingMutations,
stopObservingMutations,
setReactivityEngine,
runIfTypeOfFunction,
onAttributeRemoved,
onAttributesAdded,
closestDataStack,
Expand Down
30 changes: 8 additions & 22 deletions packages/alpinejs/src/evaluator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,6 @@ import { closestDataStack, mergeProxies } from './scope'
import { injectMagics } from './magics'
import { tryCatch, handleError } from './utils/error'

let shouldAutoEvaluateFunctions = true

export function dontAutoEvaluateFunctions(callback) {
let cache = shouldAutoEvaluateFunctions

shouldAutoEvaluateFunctions = false

let result = callback()

shouldAutoEvaluateFunctions = cache

return result
}

export function evaluate(el, expression, extras = {}) {
let result

Expand Down Expand Up @@ -49,10 +35,10 @@ export function normalEvaluator(el, expression) {
}

export function generateEvaluatorFromFunction(dataStack, func) {
return (receiver = () => {}, { scope = {}, params = [] } = {}) => {
return (receiver = () => {}, { scope = {}, params = [], autoEvaluateFunctions = true } = {}) => {
let result = func.apply(mergeProxies([scope, ...dataStack]), params)

runIfTypeOfFunction(receiver, result)
runIfTypeOfFunction(autoEvaluateFunctions, receiver, result)
}
}

Expand Down Expand Up @@ -103,7 +89,7 @@ function generateFunctionFromString(expression, el) {
function generateEvaluatorFromString(dataStack, expression, el) {
let func = generateFunctionFromString(expression, el)

return (receiver = () => {}, { scope = {}, params = [] } = {}) => {
return (receiver = () => {}, { scope = {}, params = [], autoEvaluateFunctions = true } = {}) => {
func.result = undefined
func.finished = false

Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

👍

// Once the function has run, we clear func.result so we don't create
// memory leaks. func is stored in the evaluatorMemo and every time
// it runs, it assigns the evaluated expression to result which could
Expand All @@ -126,20 +112,20 @@ function generateEvaluatorFromString(dataStack, expression, el) {
} else {
// If not, return the result when the promise resolves.
promise.then(result => {
runIfTypeOfFunction(receiver, result, completeScope, params, el)
runIfTypeOfFunction(autoEvaluateFunctions, receiver, result, completeScope, params, el)
}).catch( error => handleError( error, el, expression ) )
.finally( () => func.result = undefined )
}
}
}
}

export function runIfTypeOfFunction(receiver, value, scope, params, el) {
if (shouldAutoEvaluateFunctions && typeof value === 'function') {
export function runIfTypeOfFunction(autoEvaluateFunctions, receiver, value, scope, params, el) {
if (autoEvaluateFunctions && typeof value === 'function') {
let result = value.apply(scope, params)

if (result instanceof Promise) {
result.then(i => runIfTypeOfFunction(receiver, i, scope, params)).catch( error => handleError( error, el, value ) )
result.then(i => runIfTypeOfFunction(autoEvaluateFunctions, receiver, i, scope, params)).catch( error => handleError( error, el, value ) )
} else {
receiver(result)
}
Expand Down
6 changes: 2 additions & 4 deletions packages/alpinejs/src/utils/bind.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { dontAutoEvaluateFunctions, evaluate } from '../evaluator'
import { evaluate } from '../evaluator'
import { reactive } from '../reactivity'
import { setClasses } from './classes'
import { setStyles } from './styles'
Expand Down Expand Up @@ -180,9 +180,7 @@ export function extractProp(el, name, fallback, extract = true) {

binding.extract = extract

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

return getAttributeBinding(el, name, fallback)
Expand Down
4 changes: 2 additions & 2 deletions packages/csp/src/evaluator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function generateDataStack(el) {
}

function generateEvaluator(el, expression, dataStack) {
return (receiver = () => {}, { scope = {}, params = [] } = {}) => {
return (receiver = () => {}, { scope = {}, params = [], autoEvaluateFunctions = true } = {}) => {
let completeScope = mergeProxies([scope, ...dataStack])

let evaluatedExpression = expression.split('.').reduce(
Expand All @@ -39,7 +39,7 @@ function generateEvaluator(el, expression, dataStack) {
completeScope,
);

runIfTypeOfFunction(receiver, evaluatedExpression, completeScope, params)
runIfTypeOfFunction(autoEvaluateFunctions, receiver, evaluatedExpression, completeScope, params)
}
}

Expand Down
11 changes: 6 additions & 5 deletions packages/mask/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ export default function (Alpine) {

// We need to prevent "auto-evaluation" of functions like
// x-on expressions do so that we can use them as mask functions.
Alpine.dontAutoEvaluateFunctions(() => {
evaluator(value => {
result = typeof value === 'function' ? value(input) : value
}, { scope: {
evaluator(value => {
result = typeof value === 'function' ? value(input) : value
}, {
scope: {
// These are "magics" we'll make available to the x-mask:function:
'$input': input,
'$money': formatMoney.bind({ el }),
}})
},
autoEvaluateFunctions: false
})

return result
Expand Down
23 changes: 12 additions & 11 deletions packages/sort/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,22 @@ function generateSortHandler(expression, evaluateLater) {

return (key, position) => {
// In the case of `x-sort="handleSort"`, let us call it manually...
Alpine.dontAutoEvaluateFunctions(() => {
handle(
// If a function is returned, call it with the key/position params...
received => {
if (typeof received === 'function') received(key, position)
},
// Provide $key and $position to the scope in case they want to call their own function...
{ scope: {
handle(
// If a function is returned, call it with the key/position params...
received => {
if (typeof received === 'function') received(key, position)
},
// Provide $key and $position to the scope in case they want to call their own function...
{
scope: {
// Supporting both `$item` AND `$key` ($key for BC)...
$key: key,
$item: key,
$position: position,
} },
)
})
},
autoEvaluateFunctions: false
},
)
}
}

Expand Down
Loading