Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring using helpers.options.resolve #5965

Merged
merged 4 commits into from
Jan 8, 2019

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Jan 7, 2019

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
Copy link
Contributor

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.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

simonbrunel commented Jan 7, 2019

@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
Copy link
Member

@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
Copy link
Member

etimberg commented Jan 8, 2019

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
Copy link
Contributor Author

nagix commented Jan 8, 2019

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
Copy link
Contributor Author

nagix commented Jan 8, 2019

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
Copy link
Contributor Author

nagix commented Jan 8, 2019

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

@nagix nagix changed the title Add helpers.options._fallback Refactoring using helpers.options.resolve Jan 8, 2019
@simonbrunel
Copy link
Member

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
Copy link
Contributor Author

nagix commented Jan 8, 2019

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

simonbrunel
simonbrunel previously approved these changes Jan 8, 2019
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good, thanks @nagix

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 8, 2019
@simonbrunel
Copy link
Member

Build is failing :\

@simonbrunel
Copy link
Member

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.

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@simonbrunel simonbrunel merged commit 820debf into chartjs:master Jan 8, 2019
@nagix nagix deleted the issue-5965 branch January 8, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants