Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14707 closed defect (fixed)

[regression] Loader silently fails by default when modules 404

Reported by: Colin Snover Owned by: Rawld Gill
Priority: blocker Milestone: 1.7.2
Component: Loader Version: 1.7.1
Keywords: Cc:
Blocked By: #14750 Blocking:

Description (last modified by Colin Snover)

When someone typos a module name and it 404s there is *no* indication that this has happened (except in Chrome which reports network errors to the console by default) unless you watch the Net tab for 404s or set Firebug to report network errors to the console (it does not by default AFAIK). Dojo 1.6 had a default timeout of 15 seconds. Dojo 1.7 has a default timeout of 0 (disabled).

Change History (18)

comment:1 Changed 8 years ago by Colin Snover

Description: modified (diff)
Priority: undecidedblocker
Summary: Loader silently fails by default when modules 404[regression] Loader silently fails by default when modules 404

comment:2 Changed 8 years ago by Rawld Gill

Priority: blockerlow
Status: newassigned
Type: defectenhancement

Not outputting a warning msg to the console consequent to a programmer's error is not a blocker.

The timeout is configurable; if you want 15 seconds, then do something like this:

<script src = "path/to/dojo.jd" data-dojo-config="waitSeconds:15"></script>

I don't see how 15s is useful at all. If you're debugging a program, you're not going to wait around 15s for the program to do nothing. Why not go over to the network panel in the debugger and look for red?

My instinct is that the current code is reasonable. But I'll keep this open to let others voice opinions. If there is a strong desire to set a default wait time to something other than 0, then I'm happy to make that change.

comment:3 Changed 8 years ago by Kenneth G. Franqueiro

While I can somewhat see the "you're not going to wait around 15s" argument, I think we all might be surprised at just how effective it might be. Between gawking at the screen like "wtf", going back to your editor to try to check something, banging your head on the keyboard, etc., by the time you look at your browser again, you may very well notice the error popped up.

Personally though, I'd approach this more from the following perspectives:

  • If 1.6 had a 15s timeout, what's the harm in continuing to provide it?
  • I agree that never hearing an error out of this (at least without heading over to the net tab) is harmful to devs in the long run. Critical failures should be loud so that they can be identified as easily as possible.

comment:4 Changed 8 years ago by Kenneth G. Franqueiro

Type: enhancementdefect

Setting this back to defect, since technically it's a regression, and regression fixes are *never* enhancements...

comment:5 Changed 8 years ago by Kris Zyp

We should probably add an onerror listener for the script. The onerror event is supported in all modern browsers, and we could give a nice explicit error message in the environment that most devs work on. AFAICT, this is only unsupported in IE8 and earlier. For devs that actually dev on IE8 and earlier (poor souls), I don't really have any opinion on that.

comment:6 Changed 8 years ago by Colin Snover

Priority: lowblocker

Restoring blocker status. I do not want 1.7.2 to go out without a resolution one way or another on this ticket. Error reporting is very important.

comment:7 Changed 8 years ago by bill

Kris' onerror handler sounds like a good idea.

FYI, I tried the latest code, running in firebug (randomly tested on page dojo/tests/parser/parser.html):

>>> dojo.require("foo.bar")
undefined
"NetworkError: 404 Not Found - http://localhost/trunk/foo/bar.js"

So AFAICT there's no regression. The new require() call prints an error for me too:

>>> require(["foo/bar"], function(x){ console.log("failed"); });
function()
"NetworkError: 404 Not Found - http://localhost/trunk/foo/bar.js"

My firebugs have the "show network errors" thing selected, although I don't remember selecting it; I think it's the default.

Last edited 8 years ago by bill (previous) (diff)

comment:8 Changed 8 years ago by Rawld Gill

In [27765]:

configured loader to make extra noise upon 404 errors; refs #14707; !strict

comment:9 Changed 8 years ago by Rawld Gill

In [27765] I implemented Kris's idea which seems reasonable. I also set the default timeout to 15s as per v1.6. I'm still not convinced any of this is anything more than code noise since these errors are easily visible in the network panel.

I'll let it sit for a while and see the reaction; anybody with a strong opinion should speak up...I'm not committed to keeping these changes yet.

comment:10 Changed 8 years ago by Colin Snover

Why did you add a var xx= 0;? Obviously, I think that this change is great and will help make people feel less confused about what is going on with the new loader when it encounters an error. :)

Last edited 8 years ago by Colin Snover (previous) (diff)

comment:11 Changed 8 years ago by bill

Blocked By: 14750 added

comment:2 Changed 8 years ago by Rawld Gill

In [27783]:

removed debugging noise; refs #14707; !strict

comment:3 Changed 8 years ago by Rawld Gill

In [27786]:

rolled back improper IE hack; fixes #14750; refs #14707; !strict

comment:4 Changed 8 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed

In [27787]:

backported part of [27765]; fixes #14707; !strict

comment:5 Changed 8 years ago by Rawld Gill

In [27788]:

added trace flag for nonmodules to help out IE devs; refs #14707; !strict

comment:6 Changed 8 years ago by Colin Snover

Just wondering: would it make any sense to backport the onerror stuff as well since 1.6 and earlier running in sync mode used to report an error to the console immediately upon load failure?

comment:7 Changed 8 years ago by Rawld Gill

Not backporting the new machinery to catch script errors; that feels like too much risk for questionable reward.

Note that IE8 (probably 6 and 7) reports a loaded readystate change for a 404 error on a script element, so it is hard to detect when a script failed to load on IE. Since not all scripts/dependencies define modules, it's not as simple as just checking that all modules have arrived, and I don't want to start adding cruft to the loader for a very small user set. Therefore, IE8 and less users, you may not get a signal that a module failed to load.

In order to help this situation, I added the trace flag loader-define-nonmodule, which will console.log everytime a nonmodule is detected...as will be the case when a 404 occurs on IE when trying to load a module. Here is an example that shows how to use it:

<html>
  <head>
    <script 
		data-dojo-config="async:1, trace:{'loader-define-nonmodule':1}" 
		src="dojo/dojo.js">
	</script>
    <script>
		require(["some/nonexisting/module"], function(value){
			console.log(value);
		});
    </script>
  </head>
</html>

Upon attempting to load this in IE==8, you will see the console message:

LOG: trace:loader-define-nonmodule:some/nonexisting/module

comment:8 in reply to:  6 Changed 8 years ago by Rawld Gill

Replying to csnover:

Just wondering: would it make any sense to backport the onerror stuff as well since 1.6 and earlier running in sync mode used to report an error to the console immediately upon load failure?

You will get an XHR error in sync mode that is clearly visible in the console in IE. Other browsers will timout (in addition to the console error).

Note: See TracTickets for help on using tickets.