756542 - Implement make files to include SDK modules in builds

Status: RESOLVED FIXED

(Toolkit :: General, defect, P1)

Description

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

For more details see https://github.com/mozilla/addon-sdk/wiki/JEP-SDK-in-Firefox

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Makefile goop for copying modules is relatively simple. You can just do e.g.:

http://mxr.mozilla.org/mozilla-central/source/services/common/Makefile.in#25

(that installs the modules to resource://gre/modules/services-common)

Comment 2

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> Makefile goop for copying modules is relatively simple. You can just do e.g.:
> 
> http://mxr.mozilla.org/mozilla-central/source/services/common/Makefile.in#25
> 
> (that installs the modules to resource://gre/modules/services-common)

Yeah I just have never written or used makefile, that is why suggested someone with little bit more experience to do that instead ;)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Sure, no problem - can you attach a patch that adds the initial set of modules, just as an example? That will make it easier to see what needs to be done.

Comment 4

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

Sorry to reply so late. As of now loader module is on mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/efa8bb276e3d

Initially that will be it, to complete loader Q2 goal :)

Another module on the pipe is here (still WIP but ok for example I guess)
https://github.com/mozilla/mozilla-central/pull/4
You could get .patch for that one from
https://github.com/mozilla/mozilla-central/pull/4.patch

Let me know if you need anything else.

Comment 5

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

Gavin do you need anything else from me?

Comment 6

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

Just to be clear:

At the moment there is already module in moz-central:
https://hg.mozilla.org/mozilla-central/diff/tip/toolkit/addon-sdk/loader.js

And a patch that can be used to add another such module:
https://github.com/mozilla/mozilla-central/pull/4.patch

With that there is two modules:

toolkit/addon-sdk/loader.js
toolkit/addon-sdk/promise.js

Which are intended to appear in the builds under following uris:

resource://gre/modules/toolkit/loader.js
resource://gre/modules/toolkit/promise.js

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #6)
> Which are intended to appear in the builds under following uris:
> 
> resource://gre/modules/toolkit/loader.js
> resource://gre/modules/toolkit/promise.js

Why "/toolkit"?

Comment 8

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

Mainly because:

1. Mossop suggested resource://gre/modules/ would be confusing.
2. In sdk these modules will be located at lib/toolkit/

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Why would it be confusing? What distinction are you trying to draw between these modules and all of the rest of the modules in gre/modules? If the idea is to call them out as addon-sdk modules specifically, something like "addon-sdk" would be clearer.

Comment 10

[Dave Townsend [:mossop]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> Why would it be confusing? What distinction are you trying to draw between
> these modules and all of the rest of the modules in gre/modules? If the idea
> is to call them out as addon-sdk modules specifically, something like
> "addon-sdk" would be clearer.

The SDK modules are commonjs modules that can't be loaded with Components.utils.import (though some work as both) so I think it makes sense to have some separation from the rest of the jsm modules. Rather than creating a whole new pair of resource:// mappings though I think putting them in their own directories is enough. The main modules will be living in resource:///modules/sdk/* but the modules that are shared between apps we went for resource://gre/modules/toolkit/*. I guess there's a bunch of other names there that might make sense as alternatives, "sdk-shared" perhaps. Don't really want to bikeshed on it too much but I'm not particularly tied to the "toolkit" name. I do think we want them in a sub-dir though.

Comment 11

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

It feels to me that 'resource://gre/modules/commonjs/' may a be another good option, as only distinction from other modules there is that they are commonjs style

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #11)
> It feels to me that 'resource://gre/modules/commonjs/' may a be another good
> option, as only distinction from other modules there is that they are
> commonjs style

Yes, this makes more sense to me. Though I still feel like it will be somewhat confusing to have modules under resource://modules that aren't Cu.import()able. What are the downsides to an entirely separate mapping (resource://commonjs or somesuch)?

Comment 13

[Dave Townsend [:mossop]]

If we actually put them into different directories in the build then this becomes harder since we'll have to do more Makefile work for the copying and hack on the omnijar code to get it to include them in the jar. I don't think it's worth that extra work, developers will realise quickly when Cu.import fails that they can't use it and a descriptive name (I'm fine with commonjs too) helps explain why.

We could just map two new resource uris to the existing spaces, easy enough but I'm not sure it accomplishes much.

Comment 14

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

Gavin can we move forward with this now ?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

OK, sounds like we agree on resource://gre/modules/commonjs/. I think we should wait for bug 775338 to do this properly, it should be relatively simple after that.

In bug 774816, Paolo raises an issue with the current promise/core.js module: it seems to export all of its symbols directly, rather than namespaced in single exported symbol (Promise.*). That seems like it will be problematic for use as a JSM - can we change that?

Comment 17

[:Paolo Amadini]

Ok, so for the make rules this is waiting on bug 775338 and bug 770426, that
apparently are being worked on right now.

In the meantime, I think we can address namespacing, for the ability to work well
with the current JSM infrastructure, in particular for lazy loading.

Also, I have a specific observation on the only module that will be available,
"Promise": I propose to rename the exported "resolve" function to "resolved", and
"reject" to "rejected", because they're substantially different from the "resolve"
and "reject" functions that exist on the object returned by "defer" (bug 774816
comment 1). Users might be confused by the names and think that Promise.resolve(p)
is an alternative to deferred.resolve.

Irakli, are there specific compatibility reasons to keep the functions named that
way? If we're starting from scratch, I'd suggest renaming, however if the module
will be shared and is already used extensively elsewhere (where?) in its current
incarnation, I've no strong feelings on keeping the current names. In Task.jsm I
used "Promise.resolved(value)" and "Promise.rejected(value)" but I can always
change that.

Comment 18

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

(In reply to Paolo Amadini [:paolo] from comment #17)
> Ok, so for the make rules this is waiting on bug 775338 and bug 770426, that
> apparently are being worked on right now.
> 
> In the meantime, I think we can address namespacing, for the ability to work
> well
> with the current JSM infrastructure, in particular for lazy loading.
>


I don't mind that, we in fact never load it as JSM so I have thought about this too much and just tried to keep it as close as possible to a commonjs module semantics we have in SDK.


> 
> Also, I have a specific observation on the only module that will be
> available,
> "Promise": I propose to rename the exported "resolve" function to
> "resolved", and
> "reject" to "rejected", because they're substantially different from the
> "resolve"
> and "reject" functions that exist on the object returned by "defer" (bug
> 774816
> comment 1). Users might be confused by the names and think that
> Promise.resolve(p)
> is an alternative to deferred.resolve.
> 
> Irakli, are there specific compatibility reasons to keep the functions named
> that
> way? 


Yes API intentionally is same as in most popular [Q](https://github.com/kriskowal/q) promise library. In fact it's implements subset of Q. Users already know that API so I think keeping compatibility is important.


> If we're starting from scratch, I'd suggest renaming, however if the
> module
> will be shared and is already used extensively elsewhere (where?) in its
> current
> incarnation, I've no strong feelings on keeping the current names. In
> Task.jsm I
> used "Promise.resolved(value)" and "Promise.rejected(value)" but I can always
> change that.

We use that promise module quite intensively in SDK, although these two functions are quite exceptional anyway. That being said I still think it's best to keep compatibility with Q. Also if you want / can convince Q to change nameing as suggested I'm up for it.

Comment 19

[:Paolo Amadini]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #18)
> Yes API intentionally is same as in most popular
> [Q](https://github.com/kriskowal/q) promise library. In fact it's implements
> subset of Q. Users already know that API so I think keeping compatibility is
> important.

That's fine for me, as a reason to keep the current names. Though, I wouldn't
limit our future extensions to the Promise module to be necessarily a subset of
the Q library, if we have reasons to proceed differently.

Mentioning that we're "inspired" by Q in the module's source code could be useful
as reference, in case we need to choose names for other functions.

> We use that promise module quite intensively in SDK, although these two
> functions are quite exceptional anyway. That being said I still think it's
> best to keep compatibility with Q. Also if you want / can convince Q to
> change nameing as suggested I'm up for it.

Conversely, in the future, if the names used in Q change, whether to change the
names in our module would need to be evaluated separately :-)

Can you file a bug with a patch for namespacing, and maybe add the mentioned
source code comment?

Comment 20

[:Paolo Amadini]

From bug 775338 comment 6:
> At this point, just use JS_MODULES_PATH := $(FINAL_TARGET)/modules/commonjs,
> like browser/components/sessionstore/src/Makefile.in does.

Can we do that?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to Paolo Amadini [:paolo] from comment #20)
> From bug 775338 comment 6:
> > At this point, just use JS_MODULES_PATH := $(FINAL_TARGET)/modules/commonjs,
> > like browser/components/sessionstore/src/Makefile.in does.
> 
> Can we do that?

Yes!

Comment 22

[:Paolo Amadini]

Attachment: The patch

The makefile part seems simple after all, for the namespacing I just invented
something on top of the existing loader boilerplate :-)

Comment 23

[:Paolo Amadini]

Attachment: The actual patch

(In reply to Paolo Amadini [:paolo] from comment #22)
> The makefile part seems simple after all

...though I forgot to "hg add" the new makefiles :-)

Comment 24

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

Comment on attachment 649716 [details] [diff] [review]
The actual patch

Review of attachment 649716 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/addon-sdk/loader.js
@@ +21,5 @@
>      factory(function require(id) {
>        return globals[id];
>      }, (globals[id] = {}), { uri: document.location.href + '#' + id, id: id });
>    }
> +}).call(this, 'loader', function Loader(require, exports, module) {

I would rather use `loader` instead of `Loader` as I find it little awkward to type `Loader.Loader({ ... })` but I'm ok with it if it's conventionally correct JSM style.

::: toolkit/addon-sdk/promise/core.js
@@ +20,5 @@
>      factory(function require(id) {
>        return globals[id];
>      }, (globals[id] = {}), { uri: document.location.href + '#' + id, id: id });
>    }
> +}).call(this, 'promise/core', function Promise(require, exports, module) {

Same as above, I think I'd prefer `promise` over `Promise`

Comment 25

[:Paolo Amadini]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #24)
> I would rather use `loader` instead of `Loader` as I find it little awkward
> to type `Loader.Loader({ ... })` but I'm ok with it if it's conventionally
> correct JSM style.

Yeah, JSM exports are uppercase by convention, and I don't think these modules
require an exception (would just be difficult to remember that they're lowercase).

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 649716 [details] [diff] [review]
The actual patch

This looks fine to me, assuming it does what you guys want.

It's a bit weird that the modules end up labeled by their directory ("promise/core.js" instead of "promise.js"), but maybe that's inevitable given the weird hybrid setup we have here?

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Could we also have a test that loading module promise works?

Ideally, two tests:
- loading with Components.utils.import (from the main thread);
- loading with importScripts (from a worker thread).

I can write the tests, if necessary.

Comment 29

[:Paolo Amadini]

(In reply to David Rajchenbach Teller [:Yoric] from comment #28)
> Could we also have a test that loading module promise works?
> 
> Ideally, two tests:
> - loading with Components.utils.import (from the main thread);
> - loading with importScripts (from a worker thread).
> 
> I can write the tests, if necessary.

We should also have direct tests for the module functions (now they're tested only
indirectly). I filed bug 783987 to that effect.

If you can take the bug, or a subset of it, it's definitely appreciated. The thing
we should define, both for the functionality and the loading tests above, is
whether we should keep compatibility with the upstream Add-on SDK test structure.

Comment 30

[eundiabolo]

Comment on attachment 649716 [details] [diff] [review]
The actual patch

1

Comment 31

[eundiabolo]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #0)
> Para más detalles vea
> https://github.com/mozilla/addon-sdk/wiki/JEP-SDK-in-Firefox

Comment 32

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d24355724d6e

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d24355724d6e

Comment 34

[Dave Townsend [:mossop]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/682

Pointer to Github pull-request

Comment 35

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/8e4804cedcb7de166a06eec78a94e6be417e7607
Upstream the module changes from bug 756542.