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

.tablesorter() quietly accepts inexistent option (confusion with widgetOptions) #1319

Closed
Chealer opened this issue Nov 22, 2016 · 6 comments
Closed

Comments

@Chealer
Copy link

Chealer commented Nov 22, 2016

The .tablesorter() method directly accepts options to configure "tablesorter itself". It also accepts options to configure widgets, but only through the widgetOptions "super-option". A widget option passed as a "top option" is apparently ineffective, understandably.

Unfortunately, when one directly passes .tablesorter() a "top option" which does not exist, there is no JavaScript error, warning, or anything logged to alert the developer. I just lost numerous minutes working around the problem which this caused, and even filed ticket #1318. I am surely not alone in that situation.

The traditional approach is to give each method option a parameter, so each option given is used. The approach chosen by tablesorter is valid and probably desirable when the number of options is large, however in that case the options should be validated.

While we're at it, I would like to suggest an alternative API which does not go back to the traditional approach. The API would be naturally less error-prone if, instead of

$("#tableauGroupes").tablesorter({
			widgets: ['math'],
			widgetOptions: {
				math_priority : [ 'col', 'row', 'above', 'below' ],
			}
		}
	);

one could simply write something like:

$("#tableauGroupes").tablesorter({
			math_widget: {
                             priority : [ 'col', 'row', 'above', 'below' ]
                        }
		}
	);
@Mottie
Copy link
Owner

Mottie commented Nov 22, 2016

Hi @Chealer!

I think the documentation is pretty extensive. I don't see any advantage in adding an option validator. To me it seems that it would add unnecessary code and bloat to the core of the plugin. Anyway, even if I added a validator, you wouldn't know how to look at the output because you'd have to know about and then set the debug option to see it.

As for moving the widget options to the top level, the example still nests the widget options in one level. So the priority settings end up at the same depth in both your examples. I had toyed with the idea of moving all the widget options to the top level:

$("#tableauGroupes").tablesorter({
	widgets: ['math'],
	math_priority : [ 'col', 'row', 'above', 'below' ]
});

but then it pollutes the table.config object and make it even more unreadable. Especially when trying to debug or examine option settings. Either way, I don't plan on making these kind of changes to tablesorter as it would not be backwards compatible. I will consider some alternatives once I get more time to work on the complete rewrite of tablesorter which has been named Abelt.

@Chealer
Copy link
Author

Chealer commented Nov 26, 2016

Thank you very much @Mottie. I was not aware of the rewrite underway. Perhaps that could be more obvious, but this is interesting.

I understand your hesitation about allowing widget options at the top level.

I did not read tablesorter's code, and I imagined when filing this that there was a huge switch structure treating each option, so that a default case could be simply added. I agree that solving this would require more effort if that is not how it works. But I disagree that the user wouldn't know how to look at the output. It is OK that a debug option exists, but that does not force to be silent unless debugging is enabled. Such problems could be reported as errors or warnings.

It would be ideal to add a real validator, but short of that, it would already be a big improvement to ensure that options which do not exist are rejected. The jquery.tablesorter.js starts with a long definition of var ts and its defaults. Could a partial validator not simply verify that each option passed is part of these defaults, and trigger a warning if an option passed has no corresponding default defined?

In general, JavaScript debug-ability is low, in particular with jQuery. This wastes a lot of developer time. I must have spent about 2 hours this week debugging my tablesorter code, when I had no results but no errors. I think this is one case where this can be improved.

@Mottie
Copy link
Owner

Mottie commented Nov 26, 2016

Well, Abelt hasn't been touched in almost 2 years... I have been very short on time. I am now able to focus on my projects, but I keep getting side-tracked - mostly because of a little ADHD - because I have so much going on.

I think the easiest solution to this issue would be to create a development/debugging widget. It could check all the options without adding overhead to the core.

My highest priority right now is to resolve the pager causing Edge to crash. Once that is done, I will push out a new release. If you would like to help contribute to the documentation issues, I would be highly appreciative. And thank you again for letting me know about these concerns.

@Chealer
Copy link
Author

Chealer commented Nov 26, 2016

I see. No need for excuses, there's never enough time for anything.

I would strongly recommend making option checks opt-out (unless they are simply checked all the time). I do not remember seeing equivalent debugging widgets for JavaScript libraries and I doubt developers would think about enabling something opt-in.

Is the overhead which bothers you mostly extra code, or mostly the performance penalty of having to run that code?

@Mottie
Copy link
Owner

Mottie commented Nov 26, 2016

Hmm, you're right... adding an option validator was easier than I thought. I'll make it available in the next update.

@Chealer
Copy link
Author

Chealer commented Nov 26, 2016

Thank you very much for this rapid response, @Mottie. The solution is elegant and will surely prevent much head-banging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants