v10.10.1 Operator: `contains` regression

[VictoriaHunsaker]

Hello, I think the latest release (10.10.1) has introduced a bug in the contains operator.

Given this template:

{% assign str_var = "xyz" %}
{% assign same_str_var = "xyz" %}
{% assign diff_str_var = "abc" %}

{{ str_var contains "xy" }}
{{ str_var contains same_str_var }}
{{ str_var contains diff_str_var }}
{{ "xyz" contains str_var }}

In 10.10.0, the result is

true
true
false
true

However, in 10.10.1, the result is

false
false
false
true

[harttle]

Hi @VictoriaHunsaker , thanks for the report. I'm not able to repro the results, the latest version here yields results same as In 10.10.0 you mentioned. But I can see there's a change in contains between v10.10.0 and v10.10.1: 1937aa1f1dce01ee6332f39a6e658e85cbe4f30b

Could you provide more information:

• How did you introduce LiquidJS, via npm or via external script? For the latter, could you provide the link of the LiquidJS javascript file you're using?
• The environment you reproduced this problem. Could you provide Node.js version if in Node.js, or your UserAgent (window.navigator.userAgent) if in browser?

[benweissmann]

Hey, i'm working on the same project as @VictoriaHunsaker and dug into this a bit -- our issue is that we're passing in a wrapper value around user-provided string, to throw an explicit error if someone tries to use a non-numeric value for an operator like > or < or a filter like plus (rather than the default behavior of coercing the string to NaN). So the new check for isString(l) is introducing this regression, because we're providing an object with a indexOf() method rather than a String object:

class StrictStringForLiquid {
  private name: string;
  private value: string;

  constructor(name: string, value: string) {
    this.name = name;
    this.value = value;
  }

  toString() {
    return this.value;
  }

  valueOf() {
    const num = Number(this.value);
    if (isNaN(num)) {
      throw new Error(
        `expected ${this.name} to be a number, but it was: "${this.value}"`
      );
    }

    return num;
  }

  equals(other: unknown) {
    return this.value === String(other);
  }

  gt(other: unknown) {
    return this.valueOf() > Number(other);
  }

  geq(other: unknown) {
    return this.valueOf() >= Number(other);
  }

  lt(other: unknown) {
    return this.valueOf() < Number(other);
  }

  leq(other: unknown) {
    return this.valueOf() <= Number(other);
  }

  indexOf(other: unknown) {
    return this.value.indexOf(String(other));
  }
}

Note that Drops don't work for this use-case; the valueOf method on a Drop gets called regardless of context, whereas with a custom non-drop object like this, valueOf gets called in contexts where Liquid expects a number, and toString gets called in contexts where it expects a string.

I think you could preserve backwards-compatibility by replacing the isString(l) conditional with l && isFunction(l.indexOf)?