Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9618 closed enhancement (fixed)

[patch][cla] Enhancement: Add support for xhrGet (and ItemFileReadStore) to fail silently

Reported by: Nathan Toone Owned by: Nathan Toone
Priority: low Milestone: 1.4
Component: Core Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description

Currently xhrGet will *always* console.error out an error that comes from the server - but sometimes it is desirable to suppress that error from being displayed. dojo._getText does this by passing a fail_ok parameter - and it would be nice to have such a parameter added to xhr.js as well.

In addition, the ability to set this parameter in an IFRS would be nice too.

Attachments (3)

9618-failOK.patch (3.0 KB) - added by Nathan Toone 10 years ago.
Patch which adds this functionality
9618-failOK.2.patch (3.0 KB) - added by Nathan Toone 10 years ago.
Update to patch to fix error
IFRS_Tests.patch (1.7 KB) - added by Jared Jurkiewicz 10 years ago.

Download all attachments as: .zip

Change History (19)

Changed 10 years ago by Nathan Toone

Attachment: 9618-failOK.patch added

Patch which adds this functionality

Changed 10 years ago by Nathan Toone

Attachment: 9618-failOK.2.patch added

Update to patch to fix error

comment:1 Changed 10 years ago by James Burke

Maybe we should just use a djConfig value to on/off the console.error, defaulting to showing the error, but allowing a switch to turn it off?

What are the cases with IFRS where suppressing the console.error is helpful? Or the cases where you want selective disabling of the console.error?

comment:2 Changed 10 years ago by Nathan Toone

I had thought of using a djConfig value - but the thing is that there are times where *some* of the xhrGet calls I don't want to get errors (because I know that they might error out) - while at other times, I *do* want to have the errors displayed...so a per-xhrGet variable would be nice.

In my application, we do a "test" xhrGet on a resource - and the server returns 401 if the user is not authorized to view that resource. We key off of the errback of the xhrGet to remove the inaccessible resources from the user's view if the server deems them not to have permission for that pane.

We do a similar thing with certain data stores as well. If the fetch fails on the data store, we know that the user does not have access to that resource, and we handle it appropriately in the client. The additional error message that is always displayed by xhr (in html.js) just causes user confusion when they happen to be running firebug or viewing the console in some other application.

comment:3 Changed 10 years ago by bgbraga

+1

comment:4 Changed 10 years ago by fausto

very usefull! please on the next release! :)

comment:5 Changed 10 years ago by James Burke

toonetown: I think we can get the same effect without needing the dojo.partial, and just allowing the args object to have something like logError: false, then in the _deferError just look for dfd.ioArgs.args.logError === false. It could also be used in dojo.io.script. Does that work for you?

comment:6 Changed 10 years ago by Nathan Toone

Yeah - that should work out...except for I think that I would like to have the option be "failOk" rather than "logError" - so that we can leave the default as it is...

that is, logError would have to default to "true" to maintain current behavior...while failOk would default to false...and it would be a lot simpler check to have an option that defaults to false.

comment:7 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: newclosed

(In [19951]) Fixes #9618 - add in option for silent failing (defaults to false) to suppress the error message that gets consoled out on transport error !strict

comment:8 Changed 10 years ago by Nathan Toone

Milestone: future1.4

comment:9 Changed 10 years ago by James Burke

OK, I understand wanting a flag that defaults to false. failOk seems overly broad though -- the failure will still happen and it gets propagated through the Deferred -- the errback is still fired, so anyone listening will get the error. We just want to avoid the logging of it. Maybe skipErrorLog? That seems too long to type. I would like something shorter.

The defaulting to true for logError was just to keep the API for end users short and descriptive vs having a longer name that made the check simpler in the code.

But I'm all for a different, default false name as long as it describes what is being toggled. Just not sure if failOk is accurate enough.

comment:10 Changed 10 years ago by Nathan Toone

The "failOk" term came to me from the implementation in dojo/_base/loader/hostenv_browser.js for dojo._getText - it performs the exact same functionality - and is called "fail_ok"...

I agree that "skipErrorLog" is a bit long...but don't really have any other ideas for a default false flag.

comment:11 Changed 10 years ago by Jared Jurkiewicz

Comment: If you add options to ItemFileReadStore?, unit tests that test said options should also be provided. The changes made will only work in the declarative case, you will not be able to set that option programmatically via the constructor. More work is required.

comment:12 Changed 10 years ago by Jared Jurkiewicz

Resolution: fixed
Status: closedreopened

comment:13 Changed 10 years ago by Jared Jurkiewicz

This function also doesn't seem to work (at least not in the case of local file loads. I added a unit test specifically for this and I still see an error get logged on a failure.

See attached test in patch.

Generated console error:

PASSED test: Read API: fetch() all 0 msbootstrap.js (line 787) Object _arrayOfAllItems=[0] _arrayOfTopLevelItems=[0] Error: Deferred Cancelled: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXMLHttpRequest.send]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: file:///X:/dojo/trunk/dojo/_base/_loader/bootstrap.js :: anonymous :: line 1313" data: no] dojoType=cancel PASSED test: Read API: fetch() all failOk 0 ms

Also note the constructor of IFRS had to be changed. Without the constructor update, programmatic setting doesn't work (to my knowledge).

Changed 10 years ago by Jared Jurkiewicz

Attachment: IFRS_Tests.patch added

comment:14 Changed 10 years ago by Nathan Toone

Yes - you are correct, I missed the constructor fix there (and sorry for missing adding in a test case...) However, it *is* working as desired...in the test case, you were doing console.log(errData) - which should still be consoled out - failOk prevents the console.error that gets displayed by default in dojo.xhrGet.

comment:15 Changed 10 years ago by Nathan Toone

Resolution: fixed
Status: reopenedclosed

Test case and constructor fix checked in in [19951]

comment:16 Changed 10 years ago by Nathan Toone

(In [19990]) Fixes #9618 - add in test case (thanks Jared) and fixed constructor problem. !strict

Note: See TracTickets for help on using tickets.