Open ChrisVilches opened 1 month ago
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.
@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?
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).
I enjoy this rethought approach. Solve the problem with less code :)
@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.
Summary:
injectMagics
to make it possible to make certain plugins.dontAutoEvaluateFunctions
becauserunIfTypeOfFunction
depends on data managed by that function, butrunIfTypeOfFunction
is not exported, therefore it's impossible to make certain plugins that need to change the evaluator.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.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 ofinjectMagics
, 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 usingAlpine.data(...)
). I don't need this function for my plugins so I didn't export it, but may be for someone.