bigcommerce / paper-handlebars

Paper plugin for rendering via Handlebars.js
BSD 4-Clause "Original" or "Old" License
12 stars 36 forks source link

Reverse Helper does not work as documented #292

Open mattcoy-arcticleaf opened 1 year ago

mattcoy-arcticleaf commented 1 year ago

on the Handlebars helpers reference, reverse is listed as “reverses a string”, but if you click on it, it takes you here, which says you can reverse a string or an array. However, in Stencil, it does not appear to reverse an array. Can the helper be updated to reverse an array as well, as documented?

jairo-bc commented 1 year ago

Hi @mattcoy-arcticleaf, we can't support array there since it might have breaking compatibilities with other helpers, but we can create a new one to support array type. Also Pull Requests are always welcome!

mattcoy-arcticleaf commented 1 year ago

Hi @jairo-bc , thanks for the quick reply here!

It looks like the reverse helper as it is implemented in the handlebars-helpers library has compatibility for strings in addition to arrays now, and the code for strings is identical. I don't think anything would break by updating the one in paper-handlebars to match.

I would add a pull request to make this update, but I'm not sure where it belongs, since currently "reverse" lives under strings, but it should be moved somewhere else now?

bc-evan-johnson commented 1 year ago

It's possible someone may be relying (knowingly or unknowingly) on reverse returning undefined when the argument value is an array. Returning a reversed array could cause issues in that case.

The safest approach would be to add a new helper (which is what handlebars-helpers seems to have done, albeit with the same name - string reverse still exists, and I believe array reverse was added later).