Ticket #5398 (new enhancement)

Opened 5 months ago

Last modified 3 months ago

Syntax Error catching in dojo loader

Reported by: guest Owned by: jburke
Priority: normal Milestone: 1.2
Component: PackageSystem Version: 1.0
Severity: normal Keywords: loader debugging error debugAtAllCosts
Cc: mike@…, alex

Description

I'm submitting a patch for dojo._base._loader.loader.js

When there is a js syntax error in a package file, the loader pretty much just gives up and throws an error. Consequently that error points to the loader file, which is useless. It often does give the proper syntax error (missing a '}'). However, in the case of large files, a file that had many changes done to it, or a file that you did not work on, a line number, or a snippet of code in the erroneous area become invaluable, and saves trips to jslint.com.

I've inserted a block of code that determines if the error was due to a file not found (in which case it does nothing) or a syntax error, in which case it loads the suspect file into a script tag in the head. This file is then parsed by the browser and throws a syntax error message. Clicking on the error shows a snippet of code in that location. In IE, the correct script opens in the debugger, highlighting the line with the error.

Attachments

loader.js (21.2 kB) - added by guest 5 months ago.
loader patch
loader_test_files.zip (1.1 kB) - added by guest 5 months ago.
Loader test files. Simple 'site' with an error in required file.
syntax_errors.patch (2.0 kB) - added by guest 5 months ago.
Patch to use instead of prev attached JS file
loadererror.patch (1.5 kB) - added by jburke 3 months ago.
Patch by James to break out script attachment into firebug module.
loaderror.zip (1.6 kB) - added by jburke 3 months ago.
Test files for James' patch (unzip them in the dojo/tests/_base/_loader directory)

Change History

Changed 5 months ago by guest

loader patch

Changed 5 months ago by guest

Loader test files. Simple 'site' with an error in required file.

  Changed 5 months ago by guest

Patch submitted by Mike Wilcox

mike@…

Changed 5 months ago by guest

Patch to use instead of prev attached JS file

  Changed 5 months ago by peller

can this be moved to the debug module?

follow-up: ↓ 4   Changed 5 months ago by jburke

  • cc mike@…, alex added; alex@… removed

So how would you contrast this with the capabilities available in debugAtAllCosts? If you set debugAtAllCosts to true, then you also get this capability, since all of the modules are attached via script tags. There may be a bug with double-loading in the debugAtAllCosts case (#5091), but if that is addressed, is the current debugAtAllCosts behavior enough?

in reply to: ↑ 3   Changed 5 months ago by guest

Replying to jburke:

The thought was my patch would either work along side of it or possibly replace debugAtAllCosts.

I ran tests of DAAC on the app I'm currently developing. It's very large, with approximately 3 megs of uncompressed js and close to 200 script files. I started the timer at the top of the html and stopped it in addOnload. I turned off Firebug and other debugging. I know it's inexact, but the tests were consistent:

debugAtAllCosts: true 25375 25953 27969 26281

debugAtAllCosts: false 15141 15844 15188 16109

As you can see, and as I'm sure you already know, DAAC is very slow. That extra ten seconds (70%+) is an eternity while developing. As it is, I look for ways to shorten load time, like disabling parts of the app, so I can be more productive.

Of course there is the option where you have a hard to locate error - you could set DAAC to true and reload the app. But why wait for the time it takes to find the html file that has the DAAC switch, and reload the app. Also, the times above are not reflecting the time to find the server - another ten seconds or so. On a positive note, it's these difficulties that had me thinking of a way to improve Dojo's error catching.

So what I'm proposing is to only load a script when there is an error, and then only that script.

However, I'm far from an expert on Dojo. There may be some nuances to DAAC of which I'm unaware. I also realize I'm suggesting a change to the Dojo's base. I wouldn't expect that change to be taken lightly.

  Changed 4 months ago by guest

Update:

I've left my syntax code in Dojo for about a month now. I can testify to it's stability. Further, I've come to not only use it but absolutely depend upon it. When I work on new builds without this code and I get a syntax error not showing me where it is, I get frustrated and immediately swap in the fix.

I highly recommend that this patch be implemented. It is a simple but very effective upgrade.

  Changed 4 months ago by jburke

  • owner changed from alex to jburke

I'll evaluate this more after I finish the changes for multiple versions of dojo in a page (#4573). BTW, I expect DAAC to perform better now since I fixed a "double load" issue with DAAC in #5091). But I can still see how this patch would be useful.

Stealing the bug from Alex -- the script attachment code in this patch can be pulled out so we can reuse in the xdomain case (which has very similar looking script attachment code). Maybe get reuse in dojo.io.script too.

Changed 3 months ago by jburke

Patch by James to break out script attachment into firebug module.

Changed 3 months ago by jburke

Test files for James' patch (unzip them in the dojo/tests/_base/_loader directory)

  Changed 3 months ago by jburke

Mike,

I tried a different path for the patch, see loadererror.patch: I moved the error code to be called when the parsing (evaling) is done, and I delegate to a dojo.loadParserError() function in firebug to do script attachment work.

I did this because loader.js is supposed to be a hostenv-agnostic file: it could be running inside of Rhino, for instance. So keeping the script attachment work in loader.js will not work.

We could move the script attachment stuff into a function inside hostenv_browser.js, but already with the Dojo 1.1 changes, the built dojo.js is increasing in size, so I'm not sure yet if we want to do this, at least until we do a size scrubbing on dojo.js first.

So I put the script attachment stuff in dojo._firebug.firebug. The downside with this approach is that it only works with isDebug is true.

However, I do not want to incorporate these changes, because they do not seem to help the IE 6 and IE 7 case: I cannot get a good error telling me what file has the problem. In the test file, on some reloads, I did not even get any sort of error. It works OK in Firefox 2, Safari 3 and Opera 9.25.

So, unless we can get a solution that works for IE, I'm not inclined to include the change. Any ideas?

  Changed 3 months ago by guest

Thanks for getting back to this Jim. I've been working a lot of hours this week, so I haven't been able to re-look at the code.

I guess the firebug file is okay in order to get the patch introduced. I'm not sure why you had IE troubles - that was one of the things that was the biggest benefit. Do you have a proper IE debugger installed? Microsoft Script Debugger/Editor or something?

Mike

  Changed 3 months ago by jburke

I'm using Visual Studio 2005. However, I was not attached to the browser when I tried the test, I just had MSIE running by itself, and sometimes I get an error and sometimes not. When I did get an error, it did not give me the option to jump to the debugger.

If you have a chance, maybe try the test file zip I attached to the ticket to see if you get the same behavior, or maybe you can see how my test differs. Or maybe the patch I did was not good in some way.

I'll see if I can try again too in a couple days. Maybe I goofed my IE settings somehow.

  Changed 3 months ago by guest

Holy cow, something is weird.

I manged to get IE to throw an error every time. Sometimes it just wants to notify you of the error and not go to debugger. Unless you close and reopen the page - then it will. However, I'm not getting the right script loaded. As a matter of fact, it looks like a script from the debugger itself. I think its VB code. I'm not sure what's going on. I'll look at it more over the weekend.

Mike

  Changed 3 months ago by jburke

  • milestone changed from 1.1 to 1.2

Moving to 1.2 for now, but feel free to post more info and if we get something solid before the 1.1 beta, then we might move it back to 1.1.

Note: See TracTickets for help on using tickets.