Drop dependency on Q, use native promises

[raphinesse]

We have to do this top down in our dependency graph. That way, we always can release each package without risking to break any of its consumers that rely on Q instances being returned. The order in the following task list is a topological ordering of the below dependency graph. So if we do everything in this order, each package will be processed before any of its dependencies.

There are no plugins that depend on Q.

Task list

• [x] cordova-plugman
• [x] cordova-cli
• [x] cordova-lib
• [x] cordova-create
• [x] cordova-fetch
• Platforms
  • [x] cordova-android
  • [x] cordova-browser
  • [x] cordova-ios
  • [x] cordova-osx
  • [x] cordova-windows
• [ ] cordova-common
• [ ] cordova-coho

Cordova Dependency Graph

Only packages with outstanding work in this issue

[raphinesse]

I've added a dependency graph to make it easier to determine the correct order of doing this.

[raphinesse]

Should anyone want to tackle this for some repo, here's some of the "tools" I've been using:

List of Q instance methods:

passByCopy
join
tap
thenResolve
thenReject
isPending
isFulfilled
isRejected
spread
dispatch
get
set
del
mapply
post
send
mcall
invoke
fapply
fcall
fbind
keys
any
allResolved
allSettled
fail
progress
fin
done
timeout
delay
nfapply
nfcall
nfbind
denodeify
nbind
nmapply
npost
nsend
nmcall
ninvoke
nodeify

bash scripts that read from above list and search for usage of these methods (uses ripgrep):

# List all matches
while read -u 10 m; do
    rg "\.${m}\(";
done 10< q-methods.txt

# Print statistics on the usage count of the different methods
while read -u 10 m; do
    printf "$m: ";
    (rg -c "\.${m}\(" || echo 'x:0') | cut -d: -f2 | paste -s -d+ | bc;
done 10< q-methods.txt | grep -v ' 0$'| sort -n -k2

Naturally this is nowhere near being an automatic process as RegExs aren't powerful enough to parse JS and the code probably has a lot of .join( that has nothing to do with Q :wink: But much is solvable by simple search and replace.

[raphinesse]

Too bad nobody wrote an automated code transformation based on jscodeshift for migrating from Q to native Promises.

[brodycj]

Another thing is that I think we should get rid of superspawn as discussed in #16, rather than fixing superspawn to drop use of q. The rationale is that superspawn seems to return a non-standard Promise-like object that can also notify the listener of stdout and stderr data. I think it would be better to cleanly factor superspawn out of the Cordova packages, in a major release.

While it would be ideal to do this in the next major release, it would likely block some other desired changes such as dropping support for Node.js 6 and eventually Node.js 8.

[erisu]

@brodybits Can you elaborate on this?

it would likely block some other desired changes such as dropping support for Node.js 6 and eventually Node.js 8.

To drop Node.js 6 and 8 in the next major as planned, we only remove it from the CI testing configurations and bump the node engine in package.json. There shouldn't be much effort to this.

[brodycj]

@erisu I only wanted to give examples of some major changes that I know are wanted in an upcoming releases, and that I think should not be blocked by removing superspawn (#16). I suspect that #16 could be a major effort to implement and test on all of the various Cordova packages.

I hope this is clear now. Please feel free to ask if it is still not clear.

[erisu]

Thank you @brodybits that cleared up my confusion.

[raphinesse]

I wrote an ESLint config that should highlight all instances of Q-API usage:

const qInstanceMethodsConfig = `
    allResolved allSettled del delay denodeify dispatch done fail fapply fbind
    fcall fin invoke isFulfilled isPending isRejected mapply mcall nbind
    nfapply nfbind nfcall ninvoke nmapply nmcall nodeify npost nsend passByCopy
    post progress spread tap thenReject thenResolve timeout
`.trim().split(/\s+/).map(property => ({ property }));

const restrictedSyntaxConfig = [
    {
        selector: `
            CallExpression[arguments.length=1][arguments.0.value!=/^(,? ?|\\n)$/]
                > MemberExpression[property.name="join"][object.name!="path"][callee.object.type!="ArrayExpression"]
        `.replace(/\n/g, ''),
        message: 'possible usage of `Q.prototype.join`',
    },
    {
        selector: 'CallExpression[arguments.length=1] > MemberExpression[property.name="get"]',
        message: 'possible usage of `Q.prototype.get`',
    },
    {
        selector: 'CallExpression[arguments.length=2] > MemberExpression[property.name="set"]',
        message: 'possible usage of `Q.prototype.set`',
    },
    {
        selector: 'CallExpression > MemberExpression[property.name="any"][object.name!="jasmine"]',
        message: 'possible usage of `Q.prototype.any`',
    },
    {
        selector: 'CallExpression > MemberExpression[property.name="keys"][object.name!="Object"]',
        message: 'possible usage of `Q.prototype.keys`',
    },
    {
        selector: 'CallExpression[callee.property.name="send"]',
        message: 'possible usage of `Q.prototype.send`',
    },
]

module.exports = {
    rules: {
        'no-restricted-modules': ['error', 'q'],
        'no-restricted-properties': [
            'error', { object: 'Q' }, { object: 'q' }, ...qInstanceMethodsConfig
        ],
        'no-restricted-syntax': ['warn', ...restrictedSyntaxConfig],
    }
};