Drop dependency on Q, use native promises #710

raphinesse · 2018-09-29T09:11:31Z

Resolves Use native Promises instead of Q #681, part of Drop dependency on Q, use native promises cordova#7
I took care to make a dedicated commit for every change that wasn't a simple search-and-replace
Apart from the actual removal of Q this fixes a few problems that surfaced thereafter

Behavior of cordova/requirements should remain unchanged.

dpogue

Got a handful of comments, but I think all of them are future cleanup. Lots of weird behaviour that you accidentally drew attention to, but none of which you caused 😛

dpogue · 2018-09-29T22:35:49Z

src/cordova/requirements.js

@@ -32,21 +32,18 @@ var knownPlatforms = require('../platforms/platforms');
 *   requirements check results for each platform.
 */
 module.exports = function check_reqs (platforms) {
-    return Q().then(function () {
-        var platforms = cordova_util.preProcessOptions(platforms).platforms; // eslint-disable-line no-use-before-define
+    return Promise.resolve().then(() => {


Is there a reason to wrap this in a promise, given that the inner block is already returning Promise.all?

It's a poor-man's Promise.try.

In a nutshell: It ensures that this function always returns a Promise. Even if anything we do would cause an error to be thrown, thanks to the wrapping it will be converted to a rejection instead of bubbling up the call stack.

In this case, suppose preProcessOptions(platforms) would return undefined. Then our accessing the result's platforms property would result in an error to be thrown.

Ahh, I actually just left a comment on the cordova-fetch PR referencing Promise.try. Since we seem to use this pattern frequently, maybe it makes sense to depend on something like https://github.com/tc39/proposal-promise-try (although that isn't published to npm 😞)?

Yeah, sorry for doing a ELI5 here. And I agree with you regarding the lib: I just suggested the exact same thing in the cordova-fetch PR 😁

OTOH, as soon as Node 6 EOLs, we get a nicer Promise.try in the form of async functions. So it might not be worth the trouble beginning to micro-optimize here.

dpogue · 2018-09-29T22:39:03Z

src/cordova/restore-util.js

@@ -111,7 +110,7 @@ function installPlatformsFromConfigXML (platforms, opts) {

        // No platforms to restore from either config.xml or package.json.
        if (comboArray.length <= 0) {
-            return Q('No platforms found in config.xml or package.json. Nothing to restore');
+            return Promise.resolve('No platforms found in config.xml or package.json. Nothing to restore');


This pattern seems awkward to me, since it feels more like these should be emitted as logging events.
Does this function always resolve with a log message string, or with an array of the added platforms? Currently it does both and it would be better in terms of API to be consistent with the resolved value type.

dpogue · 2018-09-29T22:43:06Z

src/hooks/HooksRunner.js

@@ -45,7 +44,7 @@ function HooksRunner (projectRoot) {
 */
 HooksRunner.prototype.fire = function fire (hook, opts) {
    if (isHookDisabled(opts, hook)) {
-        return Q('hook ' + hook + ' is disabled.');
+        return Promise.resolve('hook ' + hook + ' is disabled.');


This method resolves with a string, or throws an error (not rejects), or resolves with the result of running scripts, or rejects with error from running scripts? 😬

Sounds about right. Powerful stuff 😁

I have some improvements for HooksRunner in the making anyway. I'll try to include a cleanup of this function's behavior too.

dpogue

👍 for merging, and we can make note of those other cleanup tasks somewhere. Some of them are probably breaking API, so we'd want to tackle them as part of a major bump (perhaps the next next major?)

raphinesse · 2018-09-30T00:27:12Z

we can make note of those other cleanup tasks somewhere

Could you take care of that please @dpogue? 🙏

Thanks a lot for the review! 🙇‍♂️

piotr-cz · 2019-03-25T18:58:53Z

Along with this change, docs for Hooks: Sample Usage should be updated

raphinesse · 2019-03-26T06:34:15Z

@piotr-cz Agreed. Actually the whole page is quite dated. I will try to fix that in the next few days.

raphinesse · 2019-03-27T19:22:31Z

@piotr-cz See apache/cordova-docs#958

raphinesse added 9 commits September 29, 2018 11:04

Replace leftover usage of Q's instance API

9c3ce13

Search and replace usage of Q's static API

ce99066

Fix linting error prefer-promise-reject-errors

86394db

Replace Q.delay with npm package delay

7681351

Use superspawn to get rid of Q.defer in plugman/install

eb9aa97

Rewrite cordova/requirements to get rid of Q.allSettled

af1a1b5

Behavior of cordova/requirements should remain unchanged.

Add ESLint rule against unhandled promises

4701dcb

Fix some unhandled promises

ddac33d

Remove Q 🎉

54a9b3d

raphinesse added the enhancement label Sep 29, 2018

raphinesse requested review from dpogue and erisu September 29, 2018 09:11

project-bot bot added this to new/unassigned/reopened prs in Apache Cordova: project-bot automation testing Sep 29, 2018

project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Sep 29, 2018

This was referenced Sep 29, 2018

Drop dependency on Q, use native promises apache/cordova#7

Open

Drop Q, use native promises apache/cordova-create#33

Merged

Drop Q, use native promises apache/cordova-fetch#49

Merged

janpio moved this from 🐣 New PR / Untriaged to ⏳ Ready for Review in Apache Cordova: Tooling Pull Requests Sep 29, 2018

dpogue reviewed Sep 29, 2018

View reviewed changes

Apache Cordova: Tooling Pull Requests automation moved this from ⏳ Ready for Review to ✅ Approved, waiting for Merge Sep 29, 2018

dpogue approved these changes Sep 29, 2018

View reviewed changes

project-bot bot moved this from new/unassigned/reopened prs to accepted prs in Apache Cordova: project-bot automation testing Sep 29, 2018

raphinesse merged commit 53b2ebb into apache:master Sep 30, 2018

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Sep 30, 2018

project-bot bot moved this from accepted prs to merged prs in Apache Cordova: project-bot automation testing Sep 30, 2018

raphinesse deleted the q-nomore branch September 30, 2018 00:26

Overman775 mentioned this pull request Mar 25, 2019

Cordova 9 caiobiodere/cordova-template-framework7-vue-webpack#152

Closed

raphinesse mentioned this pull request Mar 27, 2019

Fix hooks docs apache/cordova-docs#958

Merged

rodrigograca31 mentioned this pull request Apr 16, 2019

updated requireCordovaModule to require for non-cordova modules kelvinhokk/cordova-plugin-localization-strings#42

Merged

hiensiga mentioned this pull request May 9, 2019

Cannot build with Cordova 9.0.1 PeterHdd/cordova-plugin-crypto-file#5

Closed

ruisalgado mentioned this pull request Sep 6, 2019

Fix before_platform_add hook to be compatible with Cordova 9 kabisa/maji#279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop dependency on Q, use native promises #710

Drop dependency on Q, use native promises #710

raphinesse commented Sep 29, 2018 •

edited

Loading

dpogue left a comment

dpogue Sep 29, 2018

raphinesse Sep 29, 2018

dpogue Sep 29, 2018

raphinesse Sep 29, 2018

raphinesse Sep 29, 2018

dpogue Sep 29, 2018

dpogue Sep 29, 2018

raphinesse Sep 29, 2018 •

edited

Loading

dpogue left a comment

raphinesse commented Sep 30, 2018 •

edited

Loading

piotr-cz commented Mar 25, 2019

raphinesse commented Mar 26, 2019

raphinesse commented Mar 27, 2019

Drop dependency on Q, use native promises #710

Drop dependency on Q, use native promises #710

Conversation

raphinesse commented Sep 29, 2018 • edited Loading

dpogue left a comment

Choose a reason for hiding this comment

dpogue Sep 29, 2018

Choose a reason for hiding this comment

raphinesse Sep 29, 2018

Choose a reason for hiding this comment

dpogue Sep 29, 2018

Choose a reason for hiding this comment

raphinesse Sep 29, 2018

Choose a reason for hiding this comment

raphinesse Sep 29, 2018

Choose a reason for hiding this comment

dpogue Sep 29, 2018

Choose a reason for hiding this comment

dpogue Sep 29, 2018

Choose a reason for hiding this comment

raphinesse Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

dpogue left a comment

Choose a reason for hiding this comment

raphinesse commented Sep 30, 2018 • edited Loading

piotr-cz commented Mar 25, 2019

raphinesse commented Mar 26, 2019

raphinesse commented Mar 27, 2019

raphinesse commented Sep 29, 2018 •

edited

Loading

raphinesse Sep 29, 2018 •

edited

Loading

raphinesse commented Sep 30, 2018 •

edited

Loading