Closed Bug 552841 Opened 10 years ago Closed 9 years ago

Make the Jetpack SDK follow the CommonJS Package 1.1 standard

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: avarma, Assigned: warner)

References

()

Details

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.
It should be noted that a number of existing bugs actually contradict the CommonJS package standard, such as bug 551570.
(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.
OS: Mac OS X → All
Hardware: x86 → All
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
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
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?
Summary: Make the Jetpack SDK follow the CommonJS Package standard → Make the Jetpack SDK follow the CommonJS Package 1.0 standard
Target Milestone: -- → 0.3
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.
(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?
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. 
"""
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'.
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.
Target Milestone: 0.3 → Future
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.
Assignee: nobody → warner-bugzilla
Target Milestone: Future → --
Duplicate of this bug: 591337
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
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.
(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.
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?
Blocks: 614707
Priority: -- → P1
Whiteboard: [milestone:1.0b5]
Whiteboard: [milestone:1.0b5]
Target Milestone: --- → 1.0b5
Prospectively moving this to 1.0.

Brian: is there something here that needs to be done for 1.0?
Target Milestone: 1.0b5 → 1.0
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.
Summary: Make the Jetpack SDK follow the CommonJS Package 1.0 standard → Make the Jetpack SDK follow the CommonJS Package 1.1 standard
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.
Was fixed by https://github.com/mozilla/addon-sdk/commit/658fb89ca4d3dc4a8cfb9616ae873e979a7f3955
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.