Opened 12 years ago

Closed 8 years ago

Last modified 8 years ago

#5398 closed enhancement (fixed)

Syntax Error catching in dojo loader

Reported by: guest Owned by: Rawld Gill
Priority: high Milestone: 1.7
Component: Loader Version: 1.0
Keywords: loader debugging error debugAtAllCosts Cc: mike@…, alex, Eugene Lazutkin
Blocked By: Blocking:

Description (last modified by James Burke)

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 (5)

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

Download all attachments as: .zip

Change History (26)

Changed 12 years ago by guest

Attachment: loader.js added

loader patch

Changed 12 years ago by guest

Attachment: loader_test_files.zip added

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

comment:1 Changed 12 years ago by guest

Patch submitted by Mike Wilcox

mike@…

Changed 12 years ago by guest

Attachment: syntax_errors.patch added

Patch to use instead of prev attached JS file

comment:2 Changed 12 years ago by Adam Peller

can this be moved to the debug module?

comment:3 Changed 12 years ago by James Burke

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?

comment:4 in reply to:  3 Changed 12 years 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.

comment:5 Changed 12 years 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.

comment:6 Changed 12 years ago by James Burke

Owner: changed from alex to James Burke

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 12 years ago by James Burke

Attachment: loadererror.patch added

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

Changed 12 years ago by James Burke

Attachment: loaderror.zip added

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

comment:7 Changed 12 years ago by James Burke

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?

comment:8 Changed 12 years 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

comment:9 Changed 12 years ago by James Burke

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.

comment:10 Changed 12 years 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

comment:11 Changed 12 years ago by James Burke

Milestone: 1.11.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.

comment:12 Changed 11 years ago by James Burke

Description: modified (diff)
Milestone: 1.21.3

Moving to 1.3, but Mike Wilcox, if you have any new info, feel free to add it. I'll likely close out this ticket in the 1.3 timeframe unless there is new info.

comment:13 Changed 11 years ago by James Burke

Resolution: wontfix
Status: newclosed

Closing this out for now, but Mike can reopen if he has a new implementation that solves the IE issue.

comment:14 Changed 10 years ago by James Burke

Milestone: 1.31.4
Resolution: wontfix
Status: closedreopened

It would be nice to get something like this to work, but not optimistic, given the IE problems I mentioned before in the ticket. Going to reopen for 1.4, just to give it another look, but not sure if a solution will be found, may end up closing this again.

comment:15 Changed 10 years ago by Eugene Lazutkin

Cc: Eugene Lazutkin added

comment:16 Changed 10 years ago by James Burke

Milestone: 1.4future

Would like to pursue a module syntax that is more amenable browser debugging. Pushing this out to future.

comment:17 Changed 8 years ago by bill

Owner: changed from James Burke to Rawld Gill

not sure if these tickets are even valid anymore but assigning to rawld to check

comment:18 Changed 8 years ago by Rawld Gill

Status: reopenednew

comment:19 Changed 8 years ago by Rawld Gill

Status: newassigned

comment:20 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

With the 1.7 loader in async mode, modules are be loaded by attaching a script element. This problem should therefore be resolved.

comment:21 Changed 8 years ago by bill

Milestone: future1.7
Note: See TracTickets for help on using tickets.