552841 - Make the Jetpack SDK follow the CommonJS Package 1.1 standard

Status: RESOLVED FIXED

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

Description

[Atul Varma [:atul]]

The SDK was built to mostly follow Narwhal's package format (the CommonJS package standard wasn't ratified at the time that the SDK was written).  We should update it ASAP to support the CommonJS standard, documented here:

  http://wiki.commonjs.org/wiki/Packages/1.0

We may also want to deprecate support for the current package format.

Comment 1

[Atul Varma [:atul]]

It should be noted that a number of existing bugs actually contradict the CommonJS package standard, such as bug 551570.

Comment 2

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

(In reply to comment #1)
> It should be noted that a number of existing bugs actually contradict the
> CommonJS package standard, such as bug 551570.

If there are important reasons to deviate from the standard, we should do so.  In the case of bug 551570, however, the fix would just be a slight increase in descriptiveness, which is not worth the cost of nonconformance, so I'll wontfix that bug.

Comment 3

[Atul Varma [:atul]]

Sounds good.

It appears that part of the CommonJS package standard is that the version obey the semantic versioning standard:

  http://semver.org/

A quick glance reveals that this appears to be compatible with Mozilla's traditional versioning scheme, as outlined by nsIVersionComparator [1]. I haven't taken a close look, though, so I could be wrong.

[1] https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIVersionComparator

Comment 4

[Mikeal Rogers [:mikeal] (mrogers@)]

The semver usage in Packages is pretty contentious, right now there are two threads about it:

http://groups.google.com/group/commonjs/browse_thread/thread/c70af7ad188d6082
http://groups.google.com/group/commonjs/browse_thread/thread/a1abe3ffb7203ef4

We're currently working on the Packages/1.1 specifications which severely reduces the number of require attributes: 

http://wiki.commonjs.org/wiki/Packages/1.1

Please feel free to comment on the thread if you have issues or features that would make your life easier:

http://groups.google.com/group/commonjs/browse_thread/thread/9d714a00f5591efb

Comment 5

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

Atul and I would like to see this happen for 0.3.

However, as the 1.1 spec isn't finalized, and there is still ongoing debate about it, our current focus should be conformance with the 1.0 spec (except possibly for parts of the 1.1 spec about which all interested parties have already agreed and we can safely implement in advance of its finalization).

Brian: is this something you think you might have the time to do in the 0.3 timeframe?

Comment 6

[Mikeal Rogers [:mikeal] (mrogers@)]

There are a selection of properties that were "required" by 1.0 but have been moved to "optional" for 1.1. I would advice against enforcing those properties as "required" even if you implement to the 1.0 specification.

Comment 7

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

(In reply to comment #6)
> There are a selection of properties that were "required" by 1.0 but have been
> moved to "optional" for 1.1. I would advice against enforcing those properties
> as "required" even if you implement to the 1.0 specification.

That's good feedback, thanks!  Is there anything else in 1.1 that you consider settled enough to be worth implementing now?

Comment 8

[Mikeal Rogers [:mikeal] (mrogers@)]

There is still some debate about the rest of the spec, the "Reserved Attributes" section is the only one that I feel is settled:

"""
The following fields are reserved for future expansion: build, default, email, external, files, imports, maintainer, paths, platform, require, summary, test, using, downloads, uid. Extensions to the package descriptor specification should strive to avoid collisions for future standard names by name spacing their properties with innocuous names that do not have meanings relevant to general package management.

The following fields are reserved for package registries to use at their discretion: id, type.

All properties beginning with _ or $ are also reserved for package registries to use that their discretion. 
"""

Comment 9

[Atul Varma [:atul]]

That's good to know, thanks Mikeal!

At the very least, it looks like we need to change our use of "id" then, b/c we're currently using it to represent the em:id in install.rdf if the package is built into an extension via 'cfx xpi'.

Comment 10

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

Given that no one is working on this, it seems unlikely to make 0.3.  Retargeting to future until we can triage to a specific milestone.

Comment 11

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

Brian has expressed a willingness to tackle CommonJS compatibility issues for
the SDK 0.10 release, so assigning this bug to him.

Mikeal: I note that the CommonJS Packages 1.1 spec is still a draft, according to the wiki.  Is the discussion around it still contentious, or is it getting close to being finalized?

Unless things have changed significantly, let's continue to do what I said in comment 5, i.e. conform to the 1.0 spec modulo the parts of 1.1 that we can safely implement, particularly the recommendations in comment 6 and comment 8.

Comment 13

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

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 14

[Brian Warner [:warner :bwarner]]

Ok, I'm hearing two issues here:

1: does our "version" field conform to the (contentious anyways) "Semantic Versioning" behavior? It sounds like it does. Issue closed.

2: does the way we use the "id" key in package.json conflict with the CommonJS instructions to reserve this key for "package registries"? I'll argue that it does not conflict: we only use "id" in top-level add-on packages, not the lower-level modules/packages that are included into various addons. I.e. we only use it in the "main" package, for which there is exactly one per addon, written by the addon developer. We do not use "id" in dependency packages like "addon-kit" (which contain modules that implement useful APIs like "panel" and "xhr") which maybe used by lots of add-ons, and which are included in the Jetpack SDK. These top-level packages are unlikely to ever be published in a package registry, because they export no useful functionality for other modules: their sole purpose in life is to be executed as a top-level add-on program. (they're more like applications than libraries, in a sense).

So I think the "id" keys we put there will never be seen by a package registry, so our use of it won't conflict with anything they want to use it for. Problem solved.

Do folks agree? If so, I'll close this ticket.

Comment 15

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

(In reply to comment #14)
> Ok, I'm hearing two issues here:
> 
> 1: does our "version" field conform to the (contentious anyways) "Semantic
> Versioning" behavior? It sounds like it does. Issue closed.
> 
> 2: does the way we use the "id" key in package.json conflict with the CommonJS
> instructions to reserve this key for "package registries"? I'll argue that it
> does not conflict: we only use "id" in top-level add-on packages, not the
> lower-level modules/packages that are included into various addons. I.e. we
> only use it in the "main" package, for which there is exactly one per addon,
> written by the addon developer. We do not use "id" in dependency packages like
> "addon-kit" (which contain modules that implement useful APIs like "panel" and
> "xhr") which maybe used by lots of add-ons, and which are included in the
> Jetpack SDK. These top-level packages are unlikely to ever be published in a
> package registry, because they export no useful functionality for other
> modules: their sole purpose in life is to be executed as a top-level add-on
> program. (they're more like applications than libraries, in a sense).
> 
> So I think the "id" keys we put there will never be seen by a package registry,
> so our use of it won't conflict with anything they want to use it for. Problem
> solved.
> 
> Do folks agree? If so, I'll close this ticket.

I agree with your point but, my concern is that issues will raised when addon devs will use packages from registry that have not been written for jetpack and that will contain invalid data in jetpack scope.

Comment 16

[Brian Warner [:warner :bwarner]]

Irakli: do you mean that when a Jetpack add-on uses a (say) NPM module, the "id" property of that NPM module may confuse the jetpack code? Or other properties? I don't think we're even reading the "id" property of anything but the top-level jetpack package. Is there anything specific you think we should do to safely use NPM or other CommonJS-conforming packages?

Comment 17

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

Prospectively moving this to 1.0.

Brian: is there something here that needs to be done for 1.0?

Comment 18

[Brian Warner [:warner :bwarner]]

Myk: probably not, but I'd like to understand Irakli's concern first. If I haven't heard back in a week, I'll close it.

Comment 19

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

I think that biggest remaining problems are:

https://bugzilla.mozilla.org/show_bug.cgi?id=654983

Comment 20

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

2. Incompatible format of `main` property of `package.json`. In commonjs it's relative path in our case it's id.
3. Non existing support for 'directories.lib'.

Combination of this problems makes it impossible to write packages that may be used anywhere else other then jetpack or other way round, which IMO was the whole purpose of commonjs support.

Comment 21

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

Was fixed by https://github.com/mozilla/addon-sdk/commit/658fb89ca4d3dc4a8cfb9616ae873e979a7f3955