This is hacky https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/cuddlefish.js#L79-80 As this comment suggests we should find a better way to handle this: https://github.com/mozilla/addon-sdk/pull/172#r34018
Can y'all provide more information on the issues with the code and the benefits of implementing a better approach?
Priority: -- → P3
Target Milestone: --- → Future
I don't actually know the code there, and therefore don't know if there's a real problem. I just saw an "if (false)" and alarm bells rang.
So, the deal is that all (well, almost all) addons need to have cuddlefish.js included, because that's the loader. And cuddlefish.js first imports securable-module.js by using CI.mozIJSSubScriptLoader and .loadSubScript(). And then it locates or loads some other modules (like es5, shims, unload, plain-text-console, and memory) by asking a private copy of the sec-mod loader. securable-module pulls in self-maker too. Some of these modules might pull in other modules too: there could be a whole module graph hanging off the loader. None of these imports are done with the normal require(), because it isn't available yet, which means the manifest scanner doesn't realize that they need to be included in the (stripped) XPI. So the requirement is to tell the manifest scanner that it needs to copy these modules into the XPI even though they aren't required directly by user code (main.js or the modules it require()s). Using "if (false) require(foo);" seemed like the easiest approach: the manifest scanner already knows how to traverse all the require() statements to build the transitive-closure of needed modules, and once they're in the manifest the XPI-builder knows how to include them. The scanner isn't trying to be sophisticated enough to realize that "if (false)" means the require() will never be used (it does try to recognize comments, but even that might be trying to be too clever). The scanner's goal isn't 100% accuracy, but rather giving developers an easy way to simultaneously get symbols for external functionality into their namespace, and to declare their authority-usage intentions in a statically-analyzable way. One alternative would be to hard-code a list of required-by-loader modules in the linker, but that isn't robust against changes to those modules (perhaps when plain-text-console.js acquires a new dependency later): they could easily get out-of-sync, resulting in occasionally broken (overstripped) XPIs. Another option would be to invent a new syntax to mark these non-require() dependencies, like maybe a special kind of comment, and teach the manifest-scanner to follow those comments just like it does require() statements. Maybe "// %%HEY-LINKER-I-REQUIRE(foo)" or something. In general, "one way to do it" is better than two, but I'd go along with it if folks were more comfortable with a magic comment than an if(false) marker (which, I suppose, actually is a comment marker of sorts).
If we have a specific way to deal with this, we can reopen/refile.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.