harttle / liquidjs

A simple, expressive, safe and Shopify compatible template engine in pure JavaScript.
https://liquidjs.com
MIT License
1.52k stars 238 forks source link

Liquidjs divided_by not compatible with Ruby/Shopify Liquid #465

Closed Nowaker closed 2 years ago

Nowaker commented 2 years ago

https://shopify.github.io/liquid/filters/divided_by/

The result is rounded down to the nearest integer (that is, the floor) if the divisor is an integer.Input

{{ 16 | divided_by: 4 }} => 4
{{ 5 | divided_by: 3 }} => 1

Liquidjs:

{{ 5 | divided_by: 3 }} => 1.6666666666666667
harttle commented 2 years ago

As stated here: https://liquidjs.com/tutorials/differences.html, in JavaScript we cannot distinguish integer and float, they're just numbers. For example, I can't find a way to implement this:

{{ 5 | divided_by: 3 }} // expect 1
{{ 5.0 | divided_by: 3 }} // expect 1.6667
Nowaker commented 2 years ago

@harttle I'll provide a conforming implementation this week.

Nowaker commented 2 years ago

@harttle Using lodash:

  engine.registerFilter('divided_by', (dividend, divisor) => {
    if (_.isInteger(divisor)) {
      return _.floor(dividend, divisor)
    } else {
      return dividend / divisor
    }
  })
Nowaker commented 2 years ago

I can submit a PR at a later point (1-3 weeks) or you can use transform it to TS yourself right away. Up to you :)

harttle commented 2 years ago

The point is can you pass this test case?

{{ 5.0 | divided_by: 3 }} => 1.6666666666666667

If not, we'll need an heuristicInteger option for it's breaking.

Nowaker commented 2 years ago

@harttle Yeah, you're right... I just found this definition of divided_by broke other parts of my code.

Nowaker commented 2 years ago

Actually, your example is incorrect. The rule is if the divisor is an integer. What you want is:

{{ 5.0 | divided_by: 3 }} // 1
{{ 5 | divided_by: 3 }} // 1
{{ 5.0 | divided_by: 3.0 }} // 1.6667
{{ 5 | divided_by: 3.0 }} // 1.6667

However, my code still fails:

1 // 1
1 // 1
1 // 1.6667
1 // 1.6667

Still working on it though. I'll keep you updated.

Nowaker commented 2 years ago

Okay. There's no way to fix it on the filter level. We'd have to detect 5.0 on the parsing stage, and use our own wrapping type to differentiate between float and integer. I'm not that good with JavaScript to do it for you.

harttle commented 2 years ago

What if the divider is a variable?

I mean we can keep it simple and imperfect, then enable this detection only when heuristicInteger option is set. If this imperfect detection is useful for you.

Nowaker commented 2 years ago

...Or maybe - rather than introduce value wrapping, why don't we introduce two extra arguments that are passed to divided_by?

  engine.registerFilter('divided_by', (dividend, divisor, dividendType, divisorType) => {

with dividendType and divisorType returning float or int for informational purpose, so that divided_by can do its job.

And yes, if divider is a variable, it doesn't work. But I think it's still worth it. It gets us a little closer to Liquid compatibility.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 9.37.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

harttle commented 2 years ago

Supported another argument to enable integer arithmetic:

{{ 5 | divided_by: 3, true }}