Use "Promise.jsm" as the back-end for "promise.js"

RESOLVED FIXED

Status

Add-on SDK
General
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Paolo, Assigned: jsantell)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async:team])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
+++ 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.
(Reporter)

Updated

4 years ago
Depends on: 852411
(Reporter)

Updated

4 years ago
Depends on: 881050
Whiteboard: [Async:P1] → [Async:P1][mentor=Yoric][lang=js][only do this if you understand promises]
(Assignee)

Updated

4 years ago
Assignee: nobody → jsantell
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
Flags: needinfo?(paolo.mozmail)
Created attachment 769303 [details]
GH Pull Request 1067

Sending this your way for prelim review if there aren't any other outstanding decisions to be made
Attachment #769303 - Flags: review?(rFobic)

Comment 3

4 years ago
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.
(Reporter)

Comment 4

4 years ago
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).
Flags: needinfo?(paolo.mozmail)
Note: be sure to implement fix from bug 834756
https://github.com/mozilla/addon-sdk/pull/1077
(Reporter)

Comment 6

4 years ago
(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.
(Reporter)

Updated

4 years ago
Depends on: 834756
Comment on attachment 769303 [details]
GH Pull Request 1067

Few nits in pull.
Attachment #769303 - Flags: review?(rFobic) → review+
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?
Flags: needinfo?(dteller)
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.
Flags: needinfo?(dteller)
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.
(Assignee)

Updated

4 years ago
Blocks: 785196
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?
Flags: needinfo?(jsantell)
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)
Flags: needinfo?(jsantell)
I'm probably not needed as a mentor, then.
Whiteboard: [Async:P1][mentor=Yoric][lang=js][only do this if you understand promises] → [Async:P1]
Whiteboard: [Async:P1] → [Async:team]
(Reporter)

Updated

3 years ago
Depends on: 887923
(Assignee)

Updated

3 years ago
Depends on: 988075
(Reporter)

Updated

3 years ago
Depends on: 995184
(Reporter)

Comment 18

3 years ago
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?
Flags: needinfo?(jsantell)
Probably not up to date, will update the patch now in preparation for bug 995184's resolution
Flags: needinfo?(jsantell)
Updated patch on GH, r?ing Irakli and Erik for the UI test changes.

Also, Irakli, I added `race` to sdk/core/promises
(Assignee)

Updated

3 years ago
Attachment #769303 - Flags: review?(rFobic)
Attachment #769303 - Flags: review?(evold)
Attachment #769303 - Flags: review+
Comment on attachment 769303 [details]
GH Pull Request 1067

just nits, idc if you change that stuff or not.
Attachment #769303 - Flags: review?(evold) → review+
Updated with Erik's nits fixed
(Reporter)

Updated

3 years ago
Depends on: 996671
No longer depends on: 852411, 887923
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
No longer depends on: 881050
(Assignee)

Updated

3 years ago
Duplicate of this bug: 977395
(Assignee)

Updated

3 years ago
Duplicate of this bug: 785196
Attachment #769303 - Flags: review?(rFobic) → review+
Blocks: 998277

Comment 26

3 years ago
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
(Reporter)

Comment 27

3 years ago
(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.

Comment 28

3 years ago
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
(Reporter)

Comment 30

3 years ago
(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

Comment 33

3 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1001074
You need to log in before you can comment on or make changes to this bug.