659252 - Create non-hacky way to make xpi pull in unlinked modules.

Status: RESOLVED WONTFIX

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

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

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

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Can y'all provide more information on the issues with the code and the benefits of implementing a better approach?

Comment 2

[Dietrich Ayala (:dietrich)]

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.

Comment 3

[Brian Warner [:warner :bwarner]]

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).

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If we have a specific way to deal with this, we can reopen/refile.