#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: | [email protected]…, alex, Eugene Lazutkin |
Blocked By: | Blocking: |
Description (last modified by )
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)
Change History (26)
Changed 13 years ago by
Changed 13 years ago by
Attachment: | loader_test_files.zip added |
---|
Loader test files. Simple 'site' with an error in required file.
Changed 13 years ago by
Attachment: | syntax_errors.patch added |
---|
Patch to use instead of prev attached JS file
comment:3 follow-up: 4 Changed 13 years ago by
Cc: | [email protected]… alex added; [email protected]… 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 Changed 13 years ago by
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 13 years ago by
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 13 years ago by
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 13 years ago by
Attachment: | loadererror.patch added |
---|
Patch by James to break out script attachment into firebug module.
Changed 13 years ago by
Attachment: | loaderror.zip added |
---|
Test files for James' patch (unzip them in the dojo/tests/_base/_loader directory)
comment:7 Changed 13 years ago by
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 13 years ago by
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 13 years ago by
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 13 years ago by
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 13 years ago by
Milestone: | 1.1 → 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.
comment:12 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Milestone: | 1.2 → 1.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 12 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Closing this out for now, but Mike can reopen if he has a new implementation that solves the IE issue.
comment:14 Changed 12 years ago by
Milestone: | 1.3 → 1.4 |
---|---|
Resolution: | wontfix |
Status: | closed → reopened |
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 11 years ago by
Cc: | Eugene Lazutkin added |
---|
comment:16 Changed 11 years ago by
Milestone: | 1.4 → future |
---|
Would like to pursue a module syntax that is more amenable browser debugging. Pushing this out to future.
comment:17 Changed 10 years ago by
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 10 years ago by
Status: | reopened → new |
---|
comment:19 Changed 10 years ago by
Status: | new → assigned |
---|
comment:20 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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 9 years ago by
Milestone: | future → 1.7 |
---|
loader patch