Format numbers with delimiters

[redox]

Users in different languages will need to format the display of numbers (eg. 100000 displayed as 100,000).

We should give them an easy way to do it in their templates.

API proposal:

• Define en_EN as the default language for the number formatting. Let users override it when instanciating instantsearch through the numberLocale option.
• Define a formatNumber method that will be available both through my_instantsearch_instance.utils.formatNumber and inside Hogan templates through the {{#formatNumber}}{{my_var}}{{/formatNumber}} syntax that will format the number according to the current locale.

Examples

var search = instantsearch('latency', '6be0576ff61c053d5f9a3225e2a90f76', 'instant_search', {
  numberLocale: 'fr_FR'
});

search.addWidget(
  instantsearch.widgets.stats({
    container: '#stats',
    template: function(options) {
      return '<div>' + search.utils.formatNumber(options.nbHits) + ' results</div>';
    }
  })
);

search.addWidget(
  instantsearch.widgets.stats({
    container: '#stats2',
    template: '<div>{{#formatNumber}}{{nbHits}}{{/formatNumber}} results</div>'
  })
);

[redox]

The numberWithDelimiter use-case is the one I had in mind when I created the issue but it will be the same situation in a lot of use-cases. The real question is:

• do we want to user to pre-process (adding attributes) the hit before rendering it?
• or do we want to have a way to "filter/reformat" the values at rendering time? (like Angular.js does for instance)

[vvo]

So if every user is expecting to receive count etc as formatted numbers.

Proposal:

• provide a way to pass the user language in which we want to translate the numbers (numberLocale: 'fr_FR'. Default to 'en_EN'. See Number.toLocaleString(). It would be a global option.
• parse all received data to find numeric values and format them and automatically add $attribute_number_formatted to it
• add an option to disable the autoFormatting of numbers. Because maybe already done by the user in his own data, maybe adding attributes is not ok with him.. Etc

Doing all this work and adding attributes in a deep way (remembering the parent) should be fun.

We can try this in the instantsearch.js and if it fits move it to the helper where it should belongs I guess (everything auto formatting data seems to fit in the helper).

[pixelastic]

Adding _formatted items to the hits seems intrusive. As a user, I would not like my data being modified without me asking for it. I'd rather go the Hogan formatting way, but with a grain of what you suggest.

Proposal:

• Define the current language as en_EN, but let the user override it as a numberLocale in instantsearch.js.
• Define a Hogan function (formatNumber) available in all Hogan templates (so users can write {{#formatNumber}}{{count}}{{/formatNumber}}.
• If a user specify its template as a function returning a string, we always assume the string is Hogan-compatible and render it through Hogan

[vvo]

Adding _formatted items to the hits seems intrusive. As a user, I would not like my data being modified without me asking for it

Yes, so if done like this it should be deactivated by default

Define the current language as en_EN, but let the user override it as a numberLocale in instantsearch.js.
Define a Hogan function (formatNumber) available in all Hogan templates (so users can write {{#formatNumber}}{{count}}{{/formatNumber}}.

Excactly what I wanted to add here. 💯 for this proposal!

If a user specify its template as a function returning a string, we always assume the string is Hogan-compatible and render it through Hogan

When providing a function we do not render as hogan. Template as function is the way to provide our users "do you have another templating engine? use it!".

We can still expose search.utils.formatNumber (search here being the instantsearch instance created).

So that he can reuse it.

@pixelastic Can you udpate the issue + API proposal and do the PR? Looks cool

[pixelastic]

Do you think to auto formatting of numeric values through the _formatted-prefixed keys is still usefull if we expose the formatNumber method in all available templates?

[vvo]

Do you think to auto formatting of numeric values through the _formatted-prefixed keys is still usefull if we expose the formatNumber method in all available templates?

No we only need to do exactly what you proposed (BUT the "function return string is hogan" because function as template should not be treated as hogan).

[vvo]

To clarify:

• Define the current language as en_EN, but let the user override it as a numberLocale in instantsearch.js.
• Define a Hogan function (formatNumber) available in all Hogan templates (so users can write {{#formatNumber}}{{count}}{{/formatNumber}}.
• expose formatNumber in the instantsearch instance (maybe under the .utils namespace) so that people can use this pre-programmed function with any other templating mode

[pixelastic]

I updated the first comment (by @redox) already. I'll take this PR soon.

[vvo]

With your proposal @pixelastic (see top description) It looks like we may need to do the full named parameters instantiation right away because with:

var search = instantsearch('latency', '6be0576ff61c053d5f9a3225e2a90f76', 'instant_search', {
  numberLocale: 'fr_FR'
});

You need to remember the order of 4 arguments.
And also the current last argument is default searchParameters, which does not understand numberLocale so we might better right now do:

var search = instantsearch(
  appId: 'latency',
  apiKey: '6be0576ff61c053d5f9a3225e2a90f76',
  indexName: 'instant_search',
  numberLocale: 'fr_FR'
  defaultSearchParameters: {
    hitsPerPage: 100
  }
);

This looks to me more precise and less error prone. What do you think @algolia/javascript?

That's another issue but need to tackle it before we go public, if you agree Ill open the corresponding issue.

[pixelastic]

Oh yes, I forgot about the searchParameters. I'm ok for the option attribute when instanciating.

[pixelastic]

Where did you actually see that if was possible to define {{#formatNumber}}{{count}}{{/formatNumber}} in Hogan? The best I can do is defining a method that will have (literal) {{count}} as an input, not the real (10000) value.

I'll keep trying.

[vvo]

Strange mustache.js readme does state this example works https://github.com/janl/mustache.js/blob/master/README.md#functions

[pixelastic]

Yep, that's what I tried. But I don't have the second argument (render) and text was actually a literal {{name}} string, not Tater. I only tested quickly, though, so I might have overlooked something.

[vvo]

Maybe its a hogan issue, will see

[redox]

@jerskouille can you help, I know you're used to do that.

[vvo]

Oh yeah twitter/hogan.js#222 with comments from @jerskouille :)

[Jerska]

Yes, lambdas in Hogan have a big mismatch with the docs.
I gave the basic explanation on how I "fixed" it in the issue linked by @vvo , but here is how I used that in Shopify : https://github.com/algolia/algolia-shopify-hook/blob/master/public/shopify/assets/javascripts/algolia_helpers.js.liquid#L159-L187 .

By the way, to answer this:

As a user, I would not like my data being modified without me asking for it.

I believe it kind of is the same thing with the method, since I'm not aware of any technique to inject the method without adding it at the root of the hit object.
With Shopify, I ended up with a helpers attribute that I send with all the rendering calls.
So in the end, you'll probably end up reserving keywords at the root of the hit object, so going with _XXX doesn't seems stupid to me.
Or, a little bit more verbose inside the templates, but respecting the rule

{
  hit: hit,
  helpers: {
    formatNumber: function (text, render) { ... }
  }
}

[pixelastic]

Thanks for the debug!

So in regard with all this new info, this is what I suggest:

• _formatNumber available as a method in all Hogan templates (with the appropriate patch to Hogan templates to make that work). This way we can write {{#_formatNumber}}{{nbHits}}{{/_formatNumber}}
• data._formatNumber available when defining a template: function(data) { }.

I used the leading _ to denote that this is one of the properties we added. I could have wrapped them in a helpers object like suggested, but this made templates very verbose ({{#helpers.formatNumber}}{{nbHits}}{{/helpers.formatNumber}}.

I'll now need to pass this helper method all the way from the instantsearch instance to the render method.

[vvo]

This way, for people doing custom templates, we do not extend the data attribute.

You're right, people already have access to the instance here, no need to add the method.

[pixelastic]

But, this brings another point. I'll have to edit the current _render method of instantsearch to pass the relevant info to the render.

I could pass the numberLocale config and then manage to call utils.formatNumber with this numberLocale. Or I could pass along the formatNumber method binded with the correct locale from the start. Or directly pass the whole instantsearch instance all the way to the render.

The last one one feels a bit overkill, though.

In any case, this means editing the _render method. I don't feel like adding a fourth attribute to widget.render so maybe we should change it as named parameters also. This would require updating all the widgets, so I'd rather ask what you think first.

// Current method
 _render(helper, results, state) {
    forEach(this.widgets, function(widget) {
      if (widget.render) {
        widget.render(results, state, helper);
      }
    });
  }

// Suggested change
 _render(helper, results, state) {
   var instansearch = this;
   // or var numberLocale = this.numberLocale (en-EN)
   // or var formatNumber = this.formatNumber (method correctly bound to en-EN)
   // or var templateHelpers = this.templateHelpers (list of template methods, correctly bound)
    forEach(this.widgets, function(widget) {
      if (widget.render) {
        widget.render({
          instantsearch: instantsearch,
          // or numberLocale: numberLocal
          // or formatNumber: formatNumber
          // or templateHelpers: templateHelpers,
          results: results,
          state: state,
          helper: helper
        });
      }
    });
  }

Note that if we pass the whole instantsearch, we do not need to pass helper as it is available through instantsearch.helper. But this feels like a god object to me. I'd go with passing the templateHelpers object (list of methods that can be used in templates, bound to the correct values for this instance -like en-EN-).

[vvo]

// Suggested change
_render(helper, results, state) {
var instansearch = this;
// or var numberLocale = this.numberLocale (en-EN)
// or var formatNumber = this.formatNumber (method correctly bound to en-EN)
// or var templateHelpers = this.templateHelpers (list of template methods, correctly bound)
forEach(this.widgets, function(widget) {
if (widget.render) {
widget.render({
instantsearch: instantsearch,
// or numberLocale: numberLocal
// or formatNumber: formatNumber
// or templateHelpers: templateHelpers,
results: results,
state: state,
helper: helper
});
}
});
}

Maybe just

// Suggested change
 _render(helper, results, state) {
    forEach(this.widgets, function(widget) {
      if (widget.render) {
        widget.render({
          utils: this.utils,
          results,
          helper,
          state
        });
      }
    }, this);
  }

[vvo]

this was done and merged

[vvo]

https://github.com/algolia/instantsearch.js#template-helpers