cshaa/filtrex

Cannot create array of 1 element -> return value is element alone and not an array of a single element

Closed this issue · 5 comments

When creating an array of 1 value i.e. (42) the return value is just 42 instead of an array of a single element 42

This test passes

it('array support', () => {
        const arr = eval('(42, "fifty", pi)', {pi: Math.PI});

        expect(arr).is.array();
        expect(arr).to.be.equalTo([42, "fifty", Math.PI]);
    });

This test fails

it('array single element', () => {
        const arr = eval('(42)');

        expect(arr).is.array();
        expect(arr).to.be.equalTo([42]);
    });
cshaa commented

Hey Darren,
Yes, I am aware of this, it was a conscious design decision. The reasoning behind this is:

  1. The authors of filtrex expressions are expected to be laymen, not programmers
  2. The difference between “one thing” and “a collection containing one thing” is just way to subtle for a non-programmer

Therefore I decided that arrays containing only one object aren't something we should be encouraging. If you're expecting your user to give you an array, you can always easily wrap it yourself:

let result = eval(expr)
if (!Array.isArray(result)) { result = [ result ] }
// now the result is always an array, even if the user returns just one object

Thanks for reaching out and let me know if this solved your problem :)

Thank you for your prompt reply!

Okay.. understood that this was an explicit design decision.. and agree on the profile of the author/actor. i.e. more layman / non-programmers.

In our use case we use filtrex as a lighter-weight scripting language within a larger suite that is simpler and safer than raw JS eval().

Unfortunately for us we see some of these expressions getting quite complex and sophisticated based on some power users.. the dynamically changing the return value of the ( ... ) syntax inside of some larger expressions using array based operators yeilds some very unexpected behavior. Agree in the simple use case of (42) being just 42 for simplicity but in combination of complex expressions the changing return type causes multiple issues for us.

I respect your design decision.. we will see if we can figure out how to workaround with some pre-parsing logic to detect this case and try and coerce or prevent or fork if needed.

Thanks for your time and appreciate your work on the library!

cshaa commented

Thanks for the kind words 😁️
Another reason why one might not want to parse ( e ) as an array is because is is also the grouping/precedence operator – therefore it would break expressions like 2 * (a + b). If you decide to fork, however, it is very easy to add the traditional square-bracketed arrays – see this commit. Nonetheless, an even easier solution would be to use a custom function like this:

const extraFunctions = { array: (...args) => args };
expr = compileExpression('array(42, "fifty", pi)', { extraFunctions });
expr = compileExpression('array(42)', { extraFunctions });

Great! Already have the custom function implemented and in use.. was just one use case where the user used the ("foo") syntax that yielded unexpected results.

I think we can coach to use custom functions always for this purpose .. but I do like your suggestion on the traditional bracketed arrays as well.. this maybe a route to go for sure.. thanks for the commit reference 👍 appreciated.

cshaa commented

Aight, closing this! 👍️ But feel free to continue the discussion!