Refactoring using helpers.options.resolve

[nagix]

This PR adds helpers.options._fallback method that takes an arbitrary number of arguments and returns the first defined value as discussed in #5961.

In controller code, options such as custom.radius and custom.borderWidth don't fallback correctly when they are defined but falsy values, so this has been fixed.

Several helper methods are aliased globally, and the size of min.js is reduced by 922 bytes. Also, the variable _isPointInArea is renamed to isPointInArea as per #5937 (comment).

Edit: After some discussion, we decide to use helpers.options.resolve instead of introducing a new helper method.

[benmccann]

Wow. Thanks for doing this. Great job and very thorough!

My only question is should we make this method public. It seems like the API is relatively unlikely to change and I think it's one that we would want to use in lots of our plugins.

[kurkle]

While at it, I would alias helpers.getHoverColor too (and move it global where aliased locally).

Just something to NOT think about below this line, look at resolve instead. 😄

Some variants that come to mind:
fallbackAtIndex, fallbackNum, fallbackNumAtIndex.

And from that a generaliaztion:

	_fallback: function(fn) {
		var args = [].slice.call(arguments);
		var fnArgs = args.slice(1, fn.length - 1);
		var value = args[fn.length - 1];
		var i, ilen;
		for (i = fn.length, ilen = args.length; i < ilen; ++i) {
			value = fn.apply(null, fnArgs.concat([value, args[i]]));
		}
		return value;
	},

	_numberOrDefault: function(num, def) { return isNaN(num) ? def : num; };
	_numberAtIndexOrDefault: function(index, num, def) { 
		return _numberOrDefault(isArray(num) ? num[index] : num, isArray(def) ? def[index] : def); };

would be used like

v = _fallback(valueOrDefault, v1, v2, v3, ...);
v = _fallback(_numberAtIndexOrDefault, index, v1, v2, v3);

Note: valueAtIndexOrDefault has its arguments in wrong order for this.

[simonbrunel]

@kurkle I'm not fan of all these helpers, they are error prone because they will silently skip invalid options. For example, _numberOrDefault('foo', 42) will return 42 (with no error), but initially, the user set an incorrect value to an option with higher priority.

fallbackAtIndex is already handled by helpers.options.resolve so we should not duplicate that logic. When all options will be scriptable (helpers.options.resolve), fallbackAtIndex will become useless and I don't think there will be lot of fallback calls left in the code.

[simonbrunel]

@nagix A bit late but I'm just realizing that fallback is similar to resolve when no index or context:

fallback(a, b, c) === resolve([a, b, c])
fallbackAtIndex(???) === resolve([a, b, c], undefined, index)

Maybe we should use resolve instead, what do you think? It's a little bit less efficient but since we are going to use it for scriptable options anyway, it may be better than introducing a new method with the same logic.

You could even use it for case like:

hitRadius: !isNaN(custom.hitRadius) ? custom.hitRadius : valueAtIndexOrDefault(dataset.pointHitRadius, index, pointOptions.hitRadius)

// become

hitRadius: resolve([custom.hitRadius, dataset.pointHitRadius, pointOptions.hitRadius], undefined, index);

@benmccann and resolve is already public and heavily used by the datalabels plugin.

[etimberg]

I like the idea of using resolve here to keep the code smaller. If we do decide to have a named helper for this, then a simple implementation is below.

function fallback() {
    return resolve(Array.prototype.slice(arguments));
}

If it's not too complicated, I would just call resolve directly to avoid a loop over all the arguments for each call.

[nagix]

Considering we are going to introduce scriptable options, using resolve seems good, and its performance overhead should be limited in most cases. Because it’s simple enough, I think calling resolve directly is fine.

[nagix]

While at it, I would alias helpers.getHoverColor too (and move it global where aliased locally).

For better readability, I would move aliases to global only if they are used in more than one method/function.

[nagix]

I have made the suggested changes. The size difference from master is now -1,550 bytes.

[simonbrunel]

For better readability, I would move aliases to global only if they are used in more than one method/function.

I think I would always declare them outside to mimic partial ES6 imports:

import { valueOrDefault } from '../helpers/helpers.core';

But also because it makes easier to figure out when one is already declared, because ESLint will report a duplicated variable in this case. If locally aliased, then we could most of the time declare it multiple time locally (especially for newcomers).

[nagix]

@simonbrunel Ok, that makes sense. I will make changes shortly.

[simonbrunel]

Looks really good, thanks @nagix

[simonbrunel]

Build is failing :\

[simonbrunel]

The reason of getHoverColor to be local is that this method currently resides in core/core.helpers.js which is loaded afterward. It should work but rollup may re-order imports, thus resolve alias before importing core/core.helpers.js for side effect. I will fix this issue while finishing #4478.

[benmccann]

I like it!