Closed Bug 756542 Opened 12 years ago Closed 11 years ago

Implement make files to include SDK modules in builds

Categories

(Toolkit :: General, defect, P1)

Product:
Toolkit
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: irakli, Assigned: Paolo)

References

Details

Attachments

(1 file, 2 obsolete files)

The actual patch
5.04 KB, patch
Gavin
: review+
Details | Diff | Splinter Review 
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)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 ;)
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.
Assignee: nobody → rFobic
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.
Gavin do you need anything else from me?
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
(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"?
Mainly because:

1. Mossop suggested resource://gre/modules/ would be confusing.
2. In sdk these modules will be located at lib/toolkit/
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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
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
(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)?
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.
Gavin can we move forward with this now ?
Assignee: rFobic → gavin.sharp
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?
Blocks: 763311, 775495
Depends on: 708984
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.
(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.
(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?
Depends on: 775338
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?
(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!
Attached patch The patch (obsolete) — — Splinter Review 
The makefile part seems simple after all, for the namespacing I just invented
something on top of the existing loader boilerplate :-)
Assignee: gavin.sharp → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #649705 - Flags: review?(gavin.sharp)
Attachment #649705 - Flags: feedback?(rFobic)
Attached patch The actual patch — — Splinter Review 
(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 :-)
Attachment #649705 - Attachment is obsolete: true
Attachment #649705 - Flags: review?(gavin.sharp)
Attachment #649705 - Flags: feedback?(rFobic)
Attachment #649716 - Flags: review?(gavin.sharp)
Attachment #649716 - Flags: feedback?(rFobic)
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`
(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 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?
Attachment #649716 - Flags: review?(gavin.sharp) → review+
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.
Blocks: 783987
(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.
Attachment #649716 - Flags: feedback?(rFobic)
Comment on attachment 649716 [details] [diff] [review]
The actual patch

1
(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
Assignee: paolo.mozmail → nobody
Product: Add-on SDK → Toolkit
Version: unspecified → Trunk
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24355724d6e
Target Milestone: --- → mozilla17
Assignee: nobody → paolo.mozmail
https://hg.mozilla.org/mozilla-central/rev/d24355724d6e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 820688
Attachment #691218 - Flags: review?(rFobic)
Attachment #691218 - Attachment is obsolete: true
Attachment #691218 - Flags: review?(rFobic)
You need to log in before you can comment on or make changes to this bug.