undefined module not throwing exception when require()'d #8

Closed
pmuellr opened this Issue Jan 10, 2012 · 7 comments

Comments

Projects
None yet
4 participants

pmuellr commented Jan 10, 2012

I suspect this may only be a problem when define.unordered == true.

If you invoke require() for a module which has not been define()'d, a null is currently returned for the module exports. An exception should be thrown instead.

pmuellr added a commit to pmuellr/almond that referenced this issue Jan 10, 2012

Owner

jrburke commented Jan 18, 2012

I'm not sure this is worth the extra characters in the file -- in the interests of keeping almond small, I'm tending to just leave the code as-is, since the caller will get an undefined module anyway. I know this is not strictly as helpful as it could be, but I want to balance code weight. Going to close for now, but we can still continue the discussion and reopen if necessary.

@jrburke jrburke closed this Jan 18, 2012

pmuellr commented Jan 18, 2012

Yes, it is most definitely worth the extra characters in the file.

The problem is that this is always a failure case, unless for some reason you aren't dependent on the side effects of require()ing the module or the returned module exports. In which case, it's still a failure, since that code can be safely removed from your code path.

So, it's always a failure. Always. But, with the current version of code, developers aren't informed of the failure till they dereference the result of require(). Which might be quite distant, in both lines of code, and elapsed time, from when the require() occurred.

Now, even though I'm ashamed to admit this, I have been known, on rare occasions, to accidentally inject bugs into my code. This even happened last week! Guess what I did? I spelled the name of a module incorrectly, in a require() function. And it took me a minute or two to track down what was happening, since the null dereference occurred somewhere not close to the require() invocation.

It should have taken me no minutes to do this. I should have seen the error immediately. Woulda saved me a couple minutes!

Please read: http://martinfowler.com/ieeeSoftware/failFast.pdf

Perhaps a compromise approach would be to have a debug version of almond.js that does this check? Usually I'm less concerned about code size when I'm still in development.

Owner

jrburke commented Jan 27, 2012

@christophercurrie I think a debug version would complicate the use of almond, it is nice to just have the one file. Here are my concerns:

  1. there could be a case where for regular scripts that do not call define() are built in to the layer, and therefore the require('scriptname') would not have a define()'d value, but it is OK, because the point was to just indicate the need to load the file, since a global will be used to access the functionality. This comes out of require.js supporting loading of plain script tags.

Although, it is probably enough to say "make sure an empty define('scriptname', function(){}) is inserted for those kinds of scripts", and the r.js optimizer does this by default. There is an option to turn that off though. But anyway, this does not seem to be a big blocker, and almond should err on the side of encouraging modular development.

  1. A logical extension of what @christophercurrie is suggesting, I'm sure there are other checks that could be done, but at the cost of increasing the size of the code. So the trick is choosing the ones that would trip up the developer most and cause the worst wastes of time. This one might fit in that category.

I'll reopen this ticket for now to indicate it is still under consideration.

@jrburke jrburke reopened this Jan 27, 2012

@jrburke jrburke closed this in d541b49 Jun 1, 2012

Owner

jrburke commented Jun 1, 2012

This was partly fixed in 0.1 for dependency arrays, but was rounded out in 0.1.1.

@jrburke This behavior is inconsistent between requirejs and almond (requirejs will not throw an exception). There are some valid cases where a dependency could be undefined, such as a UMD module (https://github.com/umdjs/umd) with slightly different dependencies between AMD and CommonJS environments. The workaround suggested at jrburke#12 (comment) can be modified slightly to be an undefined module adapter, but this doesn't feel like a proper solution.

define(function () {
    return null;
});
Owner

jrburke commented Mar 24, 2013

@mikemorris I'm not sure I follow. This ticket was throwing on a module not included in the built file. Given that almond only works with all the modules built in, throwing makes sense. For requirejs, it would trigger a dynamic load. However, requirejs expects to load a value, and will throw a type of error if that dynamic load fails. It will just be an error that happens in a later turn, and could be a "timeout" error depending on how requirejs is configured.

Does this sound like the build situation: you are building something using almond, and find you need to insert a dummy module for a dependency you do not need in the almond-built version. If so, you can use the r.js stubModules to put in an empty define call for that module. But there will need to be some define() registered for that module ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment