Closed Bug 679591 Opened 13 years ago Closed 13 years ago

error when bad module name passed to require() is not helpful

Categories

(Add-on SDK Graveyard :: General, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dietrich, Assigned: warner)

References

Details

Attachments

(1 file)

It should say something that allows immediate identification of offending calling code (require function) as well as the offending argument (so you know which of your 23 require() calls is the broken one). Ideally the line number of the calling code would be printed as well. Something like "Module 'blarglesnort' was not found in your module path, so require() could not load it. Here's what it shows now: :!cfx run Using binary at '/Applications/Nightly.app/Contents/MacOS/firefox-bin'. Using profile at '/Users/dietrich/Library/Application Support/Firefox/Profiles/dwgkjdlp.jp-commands'. error: An exception occurred. Traceback (most recent call last): File "resource://jid1-xeaurv8gzibudw-at-jetpack-api-utils-lib/securable-module.js", line 402, in asyncRequire asyncMain(null, basePath, null, deps, callback); File "resource://jid1-xeaurv8gzibudw-at-jetpack-api-utils-lib/securable-module.js", line 494, in asyncMain callback TypeError: callback is undefined FAIL Total time: 1.847581 seconds Program terminated unsuccessfully. shell returned 255
Brian: I recall us discussing this before; did we ever file a bug on it? Dietrich: can you post blarglesnort somewhere public? We're all quite interested in checking it out!
Assignee: nobody → warner-bugzilla
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → 1.2
yeah, Bug 654615. But that's actually about the link-time error message when you do require("blarglesnort") and there's no blarglesnort.js on the search path. The error that Dietrich hit should only occur when you're doing something sneaky at runtime (like require("blarg"+"le"+"snort")). Dietrich: I'll second the request to see your code.. I'd like to understand how the problem got past the linker.
Hm, it might be some form of define() call, or maybe you're passing a list into require() instead of a string (which makes require() wander into the async-define code). I'm betting it's not as simple as a misspelled module name.
Party-foul: I fixed the problem locally, filed the bug, and moved on. Now I can't remember exactly what I fixed in which require() call. FWIW I'm not doing anything fancy, just require()ing core modules in my main.js. We can close this I guess, and re-open if I can reproduce?
Looking at the code, the only way I think you could have gotten that error is if you called require() without any arguments, or if the first argument was not a string and not a function and was falsey (and the second argument was undefined). We could probably add a better error message for this case ("you must provide a module name when calling require()"). OTOH, the code path you got into is meant to handle something in the async-define path, so there may be some subtle reason why it needs to tolerate deps==undefined, making this not just a trivial one-line fix. jrburke would probably know more, I think he's the original author of asyncRequire().
Speaking to Brian's comment: these kinds of define calls should be supported, where the require calls are inside the define factory function: define(function(require){ var a = require('a'); }); In that flow, inside asyncMain, deps can be set to undefined. But that path is only taken if asyncRequire is called with something that is not a string for the first argument, or there is second argument passed to require. I can lock down the code checks if there is a test/example for me to verify the fix.
Bug 551604 is about fixing the lack of tracebacks (including line numbers) in load-time exceptions, which I think would help this a lot. I'm writing a patch to make "require()" throw a "you must provide a module name" exception instead of the confusing "callback is undefined" exception (which is doubly confusing because the little traceback which *is* displayed shows only securable-module.js internals, and not the user code with the offending require() statement).
Depends on: 551604
It looks like the original exception could be caused by calling require() without any arguments, so here's a patch to catch that specific case and throw a better error. Since stack traces are currently somewhat broken (especially for exceptions that occur at the top-level of a module, where require() statements tend to live), we can't get the line number of the problem, but there's a hack in this patch to at least get the filename.
Attachment #558005 - Flags: review?(rFobic)
Comment on attachment 558005 [details] [diff] [review] emit nicer message (with filename) when require() is called without arguments Review of attachment 558005 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #558005 - Flags: review?(rFobic) → review+
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
ok, I think that message is the best we can do for now. When/if bug 551604 is fixed, we'll revisit this and see if we can improve the message.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Still eagerly anticipating the débutante ball for blarglesnort.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: