Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

v10.10.1 Operator: contains regression #675

Closed
VictoriaHunsaker opened this issue Mar 4, 2024 · 2 comments · May be fixed by bossazizan/docs#4, OATEF4313/docs#1 or Playgirlkaybraz11/docs#6
Closed

v10.10.1 Operator: contains regression #675

VictoriaHunsaker opened this issue Mar 4, 2024 · 2 comments · May be fixed by bossazizan/docs#4, OATEF4313/docs#1 or Playgirlkaybraz11/docs#6
Labels

Comments

@VictoriaHunsaker
Copy link

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
Copy link
Owner

harttle commented Mar 9, 2024

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: 1937aa1

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?

@harttle harttle added the bug label Mar 9, 2024
@benweissmann
Copy link

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)?

harttle added a commit that referenced this issue Mar 21, 2024
* fix: `contains` regression on string-like objects, #675

* chore: fix build docs on macos
github-actions bot pushed a commit that referenced this issue Mar 21, 2024
## [10.10.2](v10.10.1...v10.10.2) (2024-03-21)

### Bug Fixes

* contains regression ([#677](#677)) ([05223c4](05223c4)), closes [#675](#675)
@harttle harttle closed this as completed Apr 28, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
3 participants