Edgewall Software
Modify

Opened 9 years ago

Closed 9 years ago

#12221 closed defect (cantfix)

jquery.ready function in layout.html should catch error

Reported by: samuel.degrande@… Owned by:
Priority: normal Milestone:
Component: web frontend Version: 1.2dev
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

I use a plugin which adds a js function to be called after the document is loaded by using jquery(document).ready(function($) { … })

My function was not called.

After some investigation, I found that there was an exception thrown in the js function registered in layout.html.

The queue of functions registered to be launched after document is loaded is stopped if one of them does not return.

In my opinion, a try/catch should be added. See proposed patch.

Attachments (1)

catch_error_in_ready_function.txt (1.8 KB ) - added by anonymous 9 years ago.

Download all attachments as: .zip

Change History (10)

by anonymous, 9 years ago

comment:1 by Christian Boos, 9 years ago

Wouldn't some kind of console.log be useful in the catch clause?

comment:2 by anonymous, 9 years ago

Ah yes, sure. The purpose of the patch was just to show what part of layout.html I was talking about. Sorry, I should have been more precise in my comment.

in reply to:  description comment:3 by Ryan J Ollos, 9 years ago

Replying to samuel.degrande@univ-lille1.fr:

After some investigation, I found that there was an exception thrown in the js function registered in layout.html.

What was the exception being thrown? I'm not seeing how the code you wrapped in try/catch would raise an exception.

comment:4 by Ryan J Ollos, 9 years ago

Milestone: next-dev-1.3.x

comment:5 by anonymous, 9 years ago

In my plugin, I use some jquery UI plugins + the jquery validation plugin.

I can not use chrome.add_jquery_ui() due to an incompatibility between the validator plugin and jquery-ui-i18n on this line:

  yearSuffix: $.format(formatMonth, {month: '', year: ''})

(the validator plugin also defines a format() function)

So, in my plugin, I directly call add_script('common/js/jquery-ui.js').

It was working with Trac 1.1, but it does no more work with Trac 1.2 due to the js script that you added in layout.html: an exception is thrown because datetimepicker() is unknown.

The actual fix is obviously to now also add_script('/common/js/jquery-ui-addon.js') in my plugin.

But, as a guard against such issue, I was thinking that adding a try/catch could be a fine protection.

Last edited 9 years ago by Jun Omae (previous) (diff)

in reply to:  5 ; comment:6 by Jun Omae, 9 years ago

In my plugin, I use some jquery UI plugins + the jquery validation plugin.

I can not use chrome.add_jquery_ui() due to an incompatibility between the validator plugin and jquery-ui-i18n on this line:

  yearSuffix: $.format(formatMonth, {month: '', year: ''})

(the validator plugin also defines a format() function)

Weird. The jquery validation plugin defines $.validator.format, not $.format. See https://github.com/jzaefferer/jquery-validation/blob/1.14.0/src/core.js#L198.

So, in my plugin, I directly call add_script('common/js/jquery-ui.js').

Instead, chrome.add_jquery_ui() should be used in Trac.

But, as a guard against such issue, I was thinking that adding a try/catch could be a fine protection.

No. I don't think we should suppress errors. Suppressing the error would lead breakages in other places.

comment:7 by Jun Omae, 9 years ago

Version: 1.2dev

in reply to:  6 ; comment:8 by anonymous, 9 years ago

Replying to Jun Omae:

In my plugin, I use some jquery UI plugins + the jquery validation plugin.

I can not use chrome.add_jquery_ui() due to an incompatibility between the validator plugin and jquery-ui-i18n on this line:

  yearSuffix: $.format(formatMonth, {month: '', year: ''})

(the validator plugin also defines a format() function)

Weird. The jquery validation plugin defines $.validator.format, not $.format. See https://github.com/jzaefferer/jquery-validation/blob/1.14.0/src/core.js#L198.


I use the 1.13 version of jquery-validation, and the culprit is: https://github.com/jzaefferer/jquery-validation/blob/1.13.0/src/core.js#L1289

So, in my plugin, I directly call add_script('common/js/jquery-ui.js').

Instead, chrome.add_jquery_ui() should be used in Trac.


That's what I tried at first, and that's what failed. But updating to jquery-validation 1.14 will make me able to use it. Fine !

But, as a guard against such issue, I was thinking that adding a try/catch could be a fine protection.

No. I don't think we should suppress errors. Suppressing the error would lead breakages in other places.


Ok. But perhaps adding a console.log() in the catch as Christian proposed ?

Well, up to you, obviously. My issue can be fixed, so that's ok with me, and the ticket can be deleted !

Sorry I should have checked and found that the new jquery-validation is now compatible with jquery-ui-i18n.

in reply to:  8 comment:9 by Ryan J Ollos, 9 years ago

Resolution: cantfix
Status: newclosed

Replying to anonymous:

But, as a guard against such issue, I was thinking that adding a try/catch could be a fine protection.

No. I don't think we should suppress errors. Suppressing the error would lead breakages in other places.


Ok. But perhaps adding a console.log() in the catch as Christian proposed ?

Well, up to you, obviously. My issue can be fixed, so that's ok with me, and the ticket can be deleted !

We'd need more information: exact steps to reproduce, the error that occurs and the error that is logged in the console after the try/catch is added. The reasonable motivation for adding the try/catch would be it allowed you to locate the problem code more quickly and we'd need that to be demonstrated. Just suppressing and logging the error so that your downstream code can execute isn't a good enough reason to add the try/catch as far as I can see.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none) to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.