GitHub PullRequestReviewCommentEvent at 2014-02-10 18:47:58
Repository: addon-sdk
Actor: jsantell
Payload: {"comment":{"url":"https://api.github.com/repos/mozilla/addon-sdk/pulls/comments/9596226","id":9596226,"diff_hunk":"@@ -0,0 +1,73 @@\n+# JPM JEP\n+## Proposal\n+\n+The current CFX tool used to assist development in addons is a great utility for developing addons, but over the years, several pain points and short-comings have surfaced. With the changes to [Addon Manager](https://github.com/mozilla/addon-sdk/pull/1241) to not require a transpile step and the new `isNative` option added to [toolkit/loader](https://github.com/mozilla/addon-sdk/pull/1279) having backwards-incompatable changes, now is a good time to investigate new options for a new Jetpack tool.\n+\n+### Benefits of Change\n+\n+* **JavaScript**: JPM is written in JavaScript as opposed to Python, hopefully increasing contributions. Also assumed that developers are more likely to have a node environment set up rather than Python already. Additionally, node seems to offer much smoother installation and maintenance. We think this combination of familiarity and ease-of-use is a win for Add-on Developers.\n+* **npm**: JPM will be in a separate repository and served over npm, being in an ecosystem familiar with many JavaScript developers.\n+* **Remove Legacy Code** CFX has been around for awhile with a lot of fallbacks to support all the SDK changes throughout the years. This makes fixing some issues difficult, or impossible in the cases of having to support every option available. For example, we can get rid of the aliased modules (`require('tabs') === require('sdk/tabs')`).\n+* **Support new native Loader**: The changes in `toolkit/loader` provides node-style lookup and dynamic module requires. This is currently not compatable with how CFX checks dependencies before running.\n+* **Use new changes in AOM**: We can take advantage of no longer needing to transpile jetpack directories and injecting `bootstrap.js` and `install.rdf`, resulting in a more transparent source code, and more similar to node modules and Chrome extensions.\n+* **Explicit Opt-In from Developers**: This is the most **important** feature, IMO, as CFX will be around for some time, with it the familiarity and legacy support. The new loader changes has some backwards-incompatable changes and this should not disrupt developers who are used to the current environment that do not want to change. Those that do want to use the new loader features can explicitly opt-in by using the JPM tool, so that they are aware of the new changes and features.\n+\n+## Functionality\n+\n+[Current CFX functionality](https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/cfx).\n+\n+### New Features\n+\n+* Currently, any addon with unspecified engines, or engines supporting versions of Firefox where AOM support for native jetpacks does not exist, jpm will add a install.rdf and bootstrap.js file for backwards compatability. This can be overridden with the --force-aom flag, which will not build with these additional files. This is mainly for testing AOM support while still in development.\n+\n+### Will be Supported\n+\n+* **init**: generates a directory with all the files needed to start add-on development. Because JPM does not require a transpile step, this is mostly similar to `npm init`, which generates a `package.json`.\n+\n+* **run/test**: Installs and runs Firefox with the current add-on installed.\n+  * `-b/--binary <BINARY>`: Sets the binary of Firefox to use\n+  * `--binary-args <CMDARGS>`: Passes additional binary arguments to Firefox\n+  * `-p/--profile-dir <PROFILE>`: Start Firefox up with this profile (Should be able to take either a name or a directory)\n+  * `-o/--overload/--overload-modules`: Bake the SDK add-ons into the addon itself so it uses the local repo SDK rather than the ones built into Firefox. Should be able to consume a PATH to the SDK or use a global ENV var.\n+* **test**: Runs tests for the current add-on or the SDK\n+  * `-f/--filter <FILENAMES>:<TESTNAMES>`: Filter tests to be run by the expression. Maybe a better interface for this?\n+* **xpi**: Creates a `xpi` file of the CWD.\n+\n+### Will be Supported Eventually (not high priority)\n+\n+* `run --pkgdir`: Uses pkgdir instead of CWD.\n+* `run --addons`: Path of other addons to install. Should be able to consume a working directory or an xpi.\n+* `run --e10s`: Not currently implemented.\n+* `run --static-args`: [Passes in arguments into the addon](https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/cfx#Passing_Static_Arguments). Is this needed?\n+* `test --logfile`: Dumps a logfile of test results.\n+* `test --profile-memory`: Dumps a log of memory tests.\n+\n+### Will NOT be Supported\n+\n+* `--test-packges/--test-addons`: An alias for the old `testaddons` and `testpkgs`, do we need these to be two separate commands? A test should set up an addon on its own and be filterable with `-f` if we want to separate the two.\n+* `test --test-runner-package`: Name of package containing test-runner program. Defaults to `test-harness`. Probably should not implement this until we can run tests with any harness.\n+* `xpi --update-link, xpi --update-url`: These should probably be handled in the `package.json`.\n+* `run --keydir`: Location of signing keys.\n+* `test --times`: Runs tests x number of times.\n+* `run --extra-packages`: As dependencies will be handled via `dependencies` in the `package.json` with npm, this is not necessary.\n+* `run -g/--use-config`: This uses a `local.json` file to store arguments to be passed into jpm. [How it currently works with cfx](https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/cfx#Using_Configurations). With the new tool being written in JS, if a developer wants to do this, they can easily script it. May be useful, not not important (IMO).\n+* `run -a/--app`: Only `firefox` is currently supported, although this is needed for Fennec. Maybe we can have a better way of Fennec support.","path":"JPM.md","position":54,"original_position":54,"commit_id":"4d89be0d5a3e16bf8f2e360ec8219362ea4ebf76","original_commit_id":"4d89be0d5a3e16bf8f2e360ec8219362ea4ebf76","user":{"login":"jsantell","id":641267,"avatar_url":"https://gravatar.com/avatar/595768eaea3925799b659b979de0f93a?d=https%3A%2F%2Fidenticons.github.com%2Fc289cb85aaaac5ad8832051f0be11df1.png&r=x","gravatar_id":"595768eaea3925799b659b979de0f93a","url":"https://api.github.com/users/jsantell","html_url":"https://github.com/jsantell","followers_url":"https://api.github.com/users/jsantell/followers","following_url":"https://api.github.com/users/jsantell/following{/other_user}","gists_url":"https://api.github.com/users/jsantell/gists{/gist_id}","starred_url":"https://api.github.com/users/jsantell/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jsantell/subscriptions","organizations_url":"https://api.github.com/users/jsantell/orgs","repos_url":"https://api.github.com/users/jsantell/repos","events_url":"https://api.github.com/users/jsantell/events{/privacy}","received_events_url":"https://api.github.com/users/jsantell/received_events","type":"User","site_admin":false},"body":"The `engines` property could list multiple apps no problem, but I don't know if this line is necessary -- unless there's some sort of way to specify you're running on fennec. Right now, several fields are needed: `-a`, `-b` (for adb), `--mobile-app fennec`, `--force-mobile`. This is kind of a pain, and redundant to have all these fields. `engines` property will replace `--force-mobile`, as that just sets up the install.rdf, so maybe just having the app flag saying this is fennec, and binary for adb and should cover our bases. \r\n\r\nI'd like for jpm to just support Fennec, but I think this could be trickier than we think, and maybe support for fennec and jpm comes after the 'release' of it.","created_at":"2014-02-10T18:47:58Z","updated_at":"2014-02-10T18:47:58Z","html_url":"https://github.com/mozilla/addon-sdk/pull/1358#discussion_r9596226","pull_request_url":"https://api.github.com/repos/mozilla/addon-sdk/pulls/1358","_links":{"self":{"href":"https://api.github.com/repos/mozilla/addon-sdk/pulls/comments/9596226"},"html":{"href":"https://github.com/mozilla/addon-sdk/pull/1358#discussion_r9596226"},"pull_request":{"href":"https://api.github.com/repos/mozilla/addon-sdk/pulls/1358"}}}}