+++ This bug was initially created as a clone of Bug #810490 +++ Now that bug 810490 has landed, we can probably switch most in-tree consumers to import "Promise.jsm" directly, instead of importing "promise.js" as a JSM from the Add-on SDK. In the meantime, we should work on updating "promise.js" itself to use "Promise.jsm" as its back-end, for those consumers that use the SDK loader, or that need the additional helper functions that "promise.js" provides. This is the current module: https://github.com/mozilla/addon-sdk/blob/b51e70b4/lib/sdk/core/promise.js During the conversion to use "Promise.jsm", there are several changes we may or may not decide to do to "promise.js", to simplify the code: *Module loading boilerplate* This is the only module (in addition to the loader itself) that has its own loading boilerplate. This makes it a bit difficult to import "Promise.jsm" from inside it properly, and it's not easy to test all the cases. Maybe we could simplify the boilerplate by deciding that the module can only be loaded both as a JSM or indirectly through the loader. Other frameworks would just load this module through the SDK loader, like they must already do for all the other SDK modules. *Promises with custom prototypes* This has been designed as a means for extensibility. While this idea has its own merits, I think no SDK consumer in the wild is currently using the parameterized "defer" or the two-parameter "resolve" / "reject" calls. I think we have the occasion to remove this feature, so that we can take our time to discuss if this is the way we want to go, and in that case implement that in our promise core. Removing the feature now would allow us to use "Promise.jsm" with no wrapping, meaning we don't have to do anything special to preserve the new "promise state debuggability" feature that "Promise.jsm" provides. I think this would greatly improve the situation for SDK consumers at little or no cost. (note: per the "unstable" deprecation policy, we can decide to do this change to the SDK API without waiting, because we reasonably believe it won't affect our compatibility in practice) *Debugging of exceptions in handlers* This feature was added in bug 833877. Now that you can inspect the rejection reason on a promise from the debugger, this feature may be less important than before. However, if we think it's still useful to keep, we need to port it to the promise core, that isn't too difficult to do anyways. *Sync URL* The URL module is the only one that makes a synchronous use of promises: https://github.com/mozilla/addon-sdk/blob/b51e70b4/lib/sdk/net/url.js The "sync" option of "readURI" cannot be implemented anymore with the new promise model that complies to "Promises/A+". But it happens that the currently undocumented "readURISync" function does exactly that same thing without using promises, and again the "sync" option is probably unused, so we can easily remove the option from this experimental module.

So to clarify, for the module boilerplate, supporting only JSM and SDK/CJS loading is sufficient? Not sure of the require.js usages (and especially browsers). Confirmed with Irakli on the below, mostly notes for myself. * `readURI`'s sync option isn't used internally anywhere, and no way to do this while following A+ spec. Will update tests and docs accordingly. * Custom Prototypes: The only time a custom prototype for a deferred is used internally is in a new feature that isn't even merged yet, and that can be handled differently (or only the initial promise contains extra methods) -- propagating a custom deferred through the chains just seems weird as well. We can remove this, and if it's needed by someone in the future, we can revisit. Updating tests and docs accordingly. * Agreed on the exception debugging

Sending this your way for prelim review if there aren't any other outstanding decisions to be made

Can you do a try build with complete test coverage? This *will* break tests because some tests rely on same-tick resolution. This may also break code not covered by tests, of course. See bug 887923 for some expected failures.

The approach and the patch look good! Thank you Jordan for working on this. We're also working on switching consumers to delayed resolution individually to identify tests related to each component, like in bug 887923 (as gps mentioned), bug 881049 (for Session Restore) and bug 881050 (for Developer Tools, that is the only bug that is currently unassigned).

Note: be sure to implement fix from bug 834756 https://github.com/mozilla/addon-sdk/pull/1077

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5) > Note: be sure to implement fix from bug 834756 > https://github.com/mozilla/addon-sdk/pull/1077 Thanks for mentioning, I'm adding some dependencies to make people aware of this.

Comment on attachment 769303 [details] GH Pull Request 1067 Few nits in pull.

I would like to clarify supporting jsm and mozIJSSubScriptLoader: https://github.com/mozilla/addon-sdk/pull/1067#discussion_r5069299 Yoric, should both jsm and mozIJSSubScriptLoader access to this module be removed?

jsm access will be done through Promise.jsm I don't think that anybody has ever used mozIJSSubScriptLoader to load promise.js, so we can get rid of that, too.

I need promise.js to be loadable via mozIJSSubScriptLoader to fix bug 834756, otherwise stepping through add-on code with the debugger won't work. If the changes in this bug result in a promise implementation that is loadable using loadSubscript without the module boilerplate, then I don't mind removing that.

Note: need to expose `every` via bug 895185. Probably can alias `all` to that as well.

Hey Jordan, what's the status of this bug?

Updated with latest changes, pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=e1eed0fa6554 Changes in JP: https://github.com/mozilla/addon-sdk/pull/1067 The JP tests are now passing, so what remains is fixing up code that uses the JP promise implementation to handle cases where tests/implementations rely on promises resolving synchronously (most likely the cause for issues breaking)

List of consumers of SDK's promises: https://gist.github.com/jsantell/7621249 Hint: It's a lot. It is unlikely this patch will land before all of the above files are using Promise.jsm, or DOM Promises.

Are you still working on this, Jordan?

Myself and Benvie (mostly Benvie at this point) have been converting modules that use sdk/core/promises to Promise.jsm, as switching the underling promise implementation from sync->async could break a lot of components using it, so to migrate over to async promises (to ultimately use DOM Promises), this middle step is required. Once no one is consuming sdk/core/promises, then we can update sdk/core/promises to be async (which is prepared, just a matter of landing it)

I'm probably not needed as a mentor, then.

Once bug 995184 is resolved, mozilla-central will not have any direct use of Add-on SDK Promises anymore. Is the pull request linked by this bug up to date?

Probably not up to date, will update the patch now in preparation for bug 995184's resolution

Updated patch on GH, r?ing Irakli and Erik for the UI test changes. Also, Irakli, I added `race` to sdk/core/promises

Comment on attachment 769303 [details] GH Pull Request 1067 just nits, idc if you change that stuff or not.

Updated with Erik's nits fixed

I don't believe this depends on bug 881050 anymore, as switching devtools over to promise.jsm has no bearing on the sdk/core/promise once bug 995184 lands

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/cfc2e3373d74fb722465859fd55c1782ac68b51a Bug 881047 Use Promise.jsm as promise implementation and remove prototype inheritence promise functions. Remove net/url#readURI sync options. Promise.all now only accepts promises, not spreads. https://github.com/mozilla/addon-sdk/commit/8e646a37cb14a72f98b54d54a7a484762336cf77 Merge pull request #1067 from jsantell/use-promise-jsm Bug 881047 Use Promise.jsm as implementation for promises, r=@erikvold,@gozala

(In reply to [github robot] from comment #26) > Commits pushed to master at https://github.com/mozilla/addon-sdk Please note that this shouldn't be merged to m-c until its dependent bug 995184 lands as well.

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/eca584f4ee084f652f1a35bff337a4ebf6a5b28e Revert "Bug 881047 Use Promise.jsm as promise implementation and remove" This reverts commit cfc2e3373d74fb722465859fd55c1782ac68b51a.

Reverting merge in SDK due to errors only in Windows XP https://tbpl.mozilla.org/php/getParsedLog.php?id=38110285&tree=Jetpack

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #29) > Reverting merge in SDK due to errors only in Windows XP > https://tbpl.mozilla.org/php/getParsedLog.php?id=38110285&tree=Jetpack Are these SDK-specific errors? Now that bug 995184 has landed, we're ready to make this change in mozilla-central.

Yeah, errors only in Windows XP, most likely timing issues with the SDK

New patch, pushing to wxp try: https://tbpl.mozilla.org/?tree=Try&rev=24c77b403383

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/59731926bda8ae16960b441438a38159ec1c804b Bug 881047 Use Promise.jsm as promise implementation and remove prototype inheritence promise functions. Remove net/url#readURI sync options. Promise.all now only accepts promises, not spreads. https://github.com/mozilla/addon-sdk/commit/8caa2adc21c4f6513a75090787ffc8cd1dbe0a22 Merge pull request #1468 from jsantell/use-promise-jsm Bug 881047 Use Promise.jsm as promise implementation and remove prototype inheritence promise functions. Remove net/url#readURI sync options. Promise.all now only accepts promises, not spreads. r=@gozala,@erikvold

Tests pass on try, merging into the SDK, hopefully tests will also pass again

Windows XP opt passing now! https://tbpl.mozilla.org/?tree=Jetpack&rev=4da0e76cac3c

Closing, hopefully not having to reopen.