DataTables / DataTablesSrc

DataTables source repository
https://datatables.net
MIT License
587 stars 422 forks source link

Use `throw` is default `errMode` instead of `alert` #270

Closed PululuK closed 2 months ago

AllanJard commented 2 months ago

Thanks for the PR, however, I'm afraid I'll not pull this in. The alert is used to show the developer there is a problem that they should correct. The end user should never see the alert, and if it does happen then there is a problem the dev needs to correct.

PululuK commented 2 months ago

@AllanJard I am not sure that it is a good practice to display alerts for the end user, in addition, it is not necessarily a development error, for example if in a project we have two external modules which try to initialize a Datatable instance.

The advantage of using a throw is that we can capture these errors and display a custom message instead of displaying internal errors to the user.

It's just a question of best practices

AllanJard commented 2 months ago

It isn't a good idea to show alerts to the end user, and as I say, they should never see it, because the dev would see them and correct the problem. In my experience lots of "developers" don't see error messages in the console - or just ignore them. The alert is in their face that there is a problem that needs to be corrected before the end user gets to use the site.

For example the JSON Ajax error - if that happens the dev will see the alert and fix whatever it is. The option is available for devs who know what they are doing and want to do custom error handling or just dump it to console.

Support requests will go up if I change the default here. And I already can't cope with how many I get.

AllanJard commented 2 months ago

I forgot to address this point:

for example if in a project we have two external modules which try to initialize a Datatable instance.

[Check if it is a DataTable](https://datatables.net/reference/api/DataTable.isDataTable()) before initialising it. You wouldn't be able to set options if it is a "race" to see which one runs first.

PululuK commented 2 months ago

@AllanJard I understand but I'm not sure you have the responsibility to teach programming to others in this context. It is best to follow best practices, regardless of whether others are not following them.

PululuK commented 2 months ago

I forgot to address this point:

for example if in a project we have two external modules which try to initialize a Datatable instance.

[Check if it is a DataTable](https://datatables.net/reference/api/DataTable.isDataTable()) before initialising it. You wouldn't be able to set options if it is a "race" to see which one runs first.

this was just an example to indicate that the error may not necessarily come from our code, but from another external module, and we should not pay for the implementation error of others

AllanJard commented 2 months ago

I don't really see the problem with having it as alert() - it just makes the error more obvious. If it throws on the console, there is still an error and it needs to be dealt with. In fact it might be a good thing because it makes the dev actually fix the issue rather than just ignoring the error on the console.

I'm afraid I will not be making this change.

PululuK commented 2 months ago

@AllanJard This is not a valid reason. It's not professional to deploy a library in production that potentially risks displaying internal errors to end users.

PululuK commented 2 months ago

I don't really see the problem with having it as alert() - it just makes the error more obvious. If it throws on the console, there is still an error and it needs to be dealt with. In fact it might be a good thing because it makes the dev actually fix the issue rather than just ignoring the error on the console.

I'm afraid I will not be making this change.

Displaying alerts in JavaScript code can have several drawbacks:

AllanJard commented 2 months ago

But that's my point. The end user should never see it. If they do, the dev has missed something that they need to correct. The alert makes it clear to the dev that they need to fix something.

If there is a specific situation where you think the alert shouldn't be shown, please link to a test case. However, for things like Ajax errors it absolutely should be there so the dev can see it and fix it before the end user sees it.

PululuK commented 2 months ago

@AllanJard Capture d’écran du 2024-04-18 15-15-19

AllanJard commented 2 months ago

Right - but that's a development error. Even if that were just dumped on the console it would need to be fixed (I guess it could be ignored, but...). DataTables is telling you something has gone wrong and it needs to be addressed. In this cause you could use DataTable.isDataTable() to check if it is a DataTable before initialising. Or rejig the code to not initialise the DataTable multiple times.

PululuK commented 2 months ago

I think I have given you enough professional arguments to explain my point of view, but you seem to be acting in bad faith. We are going to turn to another more professional plugin. Thank you all the same for your work! Have a good day

AllanJard commented 2 months ago

We'll agree to disagree :-). From my point of view it sounds like you don't want to fix the condition that leads to this error, and yet you question my professionalism. If you want to dump the error on the console instead of showing the alert and ignore it, that's totally possible with DataTable.ext.errMode = 'throw'; on your page.