search

TomFrost / Jexl

Javascript Expression Language: Powerful context-based expression parser and evaluator
MIT License
561 stars 92 forks source link

Add synchronous eval() -> syncEval() #46

Closed thettler closed 5 years ago

thettler commented 5 years ago

Hey @TomFrost, Thank you for your work on Jexel. It's a great package.

To the PR: I have added a syncEval () method which allows to execute expressions synchronously as well. An example why this can be important was given in #40. The code has no impact on existing functionality or interfaces.

I tried to keep the code duplication as low as possible and added tests.

I hope this helps you. Kind regards

Tobias Hettler

thettler commented 5 years ago

The problem with the duplication in the tests is that syncEval() and eval() are tested in the same test cases, so the setup is always the same.

To avoid this, a separate function would have to be created for each setup, which will be shared between the two test cases. That would be a lot of functions ...I did not like that either.

Of course you could also do the assertions for the sync case with the async case in one test. So you would have to do the setup only once but would test two different things in one test.

I would be happy about suggestions on how to solve this :)

TomFrost commented 5 years ago

This is a super interesting approach to the problem. I was toying with a fully synchronous Promise implementation that fakes the Promise.(resolve/reject/then/catch/etc) API and just calls all callbacks synchronously, then passing either the native Promise or the synchronous Promise around to each function depending on what mode was called.

Your solution is more direct and offers better control over how sync vs async works, mine results in less code to maintain and not as much logic duplication, but passing Promise around is obtuse. Definite pros and cons to each.

I need a bit more time to play with this, then I'll make a call and get something released. Thanks for submitting this!

TomFrost commented 5 years ago

I played around a bit more and realized the whole thing could be simplified by making the Promise implementation a member of the Evaluator class rather than passing it around -- with that change, I think it'll be easier to maintain my original solution going forward.

Thanks so much for submitting this, though, as it definitely pushed me to make a decision and get this functionality out. If you'd like to look at my approach, check out #48.