Proposal: improve `connectRange()` behavior

[francoischalifour]

Improve the refine() documentation

When you create a calendar widget (to refine with date ranges), you sometimes need to specify a lower bound without any upper bound (let's say "Display all events from 26/01/2018"). A way to do so is to pass undefined as the upper bound:

refine([start, undefined])

I don't think we've documented passing undefined anywhere (see refine() doc).

Actions:

• Document that refine() accepts undefined as an unlimited bound

Consider supporting Infinity

For now, the refine() method only takes finite values. Passing -Infinity or Infinity to a range made sense at first to me (typeof Infinity === 'number'). It is however ignored by refine() and doesn't trigger any refinements when provided. I think it should be supported since it is a number and should fall back to undefined internally.

The documentation doesn't state that refine() only supports finite numbers.

Actions:

• Specify in the refine() doc that it filters only with finite values OR
• Support infinite values (process them as undefined internally)
• (?) Log warnings when the refinement isn't triggered

Future API improvements

As discussed with @samouss, it might be interesting to change refine(Array<number, number>) to an object refine({ min: number, max: number}) in a next major release. A reason behind this is that both values are optional. It also feels more natural to pass an object rather than a tuple (see range). If we decide to do so, we should also change the start method to a similar object.

---

Do you think these inputs are relevant? Should we take action?

TL;DR

• refine() accepting undefined is not documented
• refine() only accepts finite values (not Infinity and -Infinity)
• Infinity and -Infinity should behave like undefined
• We should consider passing an object to refine() instead of an array in the future

Related

• #2309

[samouss]

First of all, thanks for the propositions!

refine() accepting undefined is not documented

Agree we should document that.

Infinity and -Infinity should behave like undefined

Adding the support for Infinity doesn't make much sense to me. Right now when you pass a value to the refine function it's either a finite number or undefined. At the end when you pass a number the client will query the API with the provided value. When you omit the value (aka undefined) the message is clear you didn't want to pass the value to the API. As a user with Infinity I might expect that since I'm passing a value, it will be send to the API. But at the end it will not be the case. I agree that the current signature with the tuple isn't very intuitive [undefined, 1000] but with an object the intent is way clearer { max: 1000 }.

[bobylito]

@samouss I think the support for Infinity is linked to the fact that it is provided in the base values (start) that as an implementer you want to reuse because it is previous state and you only ever update one part of the range per user interaction.

This start variable shouldn't contain values that are not compatible with refine. Therefore, another option could be that the start contains only number|undefined values.

[bobylito]

The tuple vs object problem is mainly good for consistency as we have now two kinds of structure for the same kind of values and it has a slightly better readability, even though it is less explicit. I'm ok to do the change in the next major version.

[samouss]

@bobylito Yes but IIRC when we worked on the connectRange recently we kept the Infinity value only for backward compatibility. At some point we should get rid of the Infinity initialisation for undefined. Because inside the rendering we don't use those values, we transform them before the component creation.

https://github.com/algolia/instantsearch.js/blob/35a1da173a99decec253aa985b2ff8e60f9c872a/src/widgets/range-input/range-input.js#L42-L45

https://github.com/algolia/instantsearch.js/blob/3b659783cd659f82539d97aec2e3ca1e691797f5/src/widgets/range-slider/range-slider.js#L43-L53

[francoischalifour]

Both arguments for supporting (or not) Infinity are valid. Dropping or adding complete support for infinite values is something we should consider in a next major release if it is to add breaking changes then.

I'm going to document the usage of undefined as a "no bound" value for refine() since this is a priority 🙂

[bobylito]

@samouss I agree with you that eventually we should get rid of the references to Infinity. However in the mean time, this is a value provided by the API (start) so we ought to support it, to make sure that the noop works: refine(start).